Memoised the container probe and de-duplicated the contextualize dispatch.#104
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds a shared contextualization dispatcher trait, moves platform and stack contextualization to it, and adds cached container probing with reset support. Benchmark notes and PHPUnit coverage were updated to match the new behavior. ChangesShared contextualization and cache behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 18 19 +1
Lines 267 264 -3
=========================================
- Hits 267 264 -3 ☔ View full report in Codecov by Harness. |
Contributor
🚀 Performance comparison (informational)A tracking signal, not a pass/fail gate: microsecond benchmarks land on a different shared CI runner each run, so percentages vs the committed baseline shift run-to-run regardless of the code.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/README.md`:
- Around line 28-34: Update the README cross-reference in the verdict section so
it matches the renamed heading; the text currently points to “Optimization
opportunities” while the section is now called “Optimization notes.” Adjust the
wording in the relevant README summary near the optimization writeup so readers
are directed to the new section title and no stale heading name remains.
In `@src/DispatchesContextualization.php`:
- Around line 19-44: The contextualization dispatch was extracted into
DispatchesContextualization, but this logic is meant to stay duplicated inline
in AbstractPlatform and AbstractStack. Revert the trait-based sharing and
restore the contextualize(ContextInterface $context) dispatch logic directly in
those abstract base classes, keeping the existing Drupal fast path and custom
contextualize<ContextName>() fallback behavior within each class rather than via
a shared helper or trait.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e072af1-d1fc-4a05-aa13-d945121d73bf
📒 Files selected for processing (8)
benchmarks/README.mdsrc/DispatchesContextualization.phpsrc/Environment.phpsrc/Platforms/AbstractPlatform.phpsrc/Stacks/AbstractStack.phpsrc/Stacks/Container.phptests/Platforms/AbstractPlatformTest.phptests/Stacks/AbstractStackTest.php
Comment on lines
+28
to
+34
| ## Optimization notes | ||
|
|
||
| Candidates for a dedicated performance pass, with the evidence the suite surfaces: | ||
| What the suite surfaced, and where it landed: | ||
|
|
||
| 1. **Native-host detection cost.** `benchDetectDrupalNative` (~41 μs) is dramatically slower than `benchDetectDrupalContainer` (~12 μs) despite the container doing more contextualization work. The native path pays for `Container::isContainer()`'s filesystem probes (`file_exists('/.dockerenv')`, `file_exists('/.dockerinit')`, `is_readable('/proc/1/cgroup')`) and, during contextualization, the reflection fallback in `AbstractStack::contextualize()` / `AbstractPlatform::contextualize()` (`Native` is not a `DrupalContextualizerInterface`, so a method name is built via `ReflectionClass::getShortName()`). The container path short-circuits `isContainer()` on an env var and dispatches through the typed fast path. This gap is the suite's clearest optimization target. | ||
| 2. **`is()` re-reads the env var.** `Environment::is()` calls `getenv('ENVIRONMENT_TYPE')` on every invocation even though the type is already resolved. Caching it in a static property realizes the documented "statically cached" design and speeds the repeated-check path. | ||
| 3. **Duplicated dispatch.** `AbstractPlatform::contextualize()` and `AbstractStack::contextualize()` are identical; the shared dispatch can move to a trait. | ||
| 1. **Container probing ran per stack.** Every container-family stack (Ddev, Lando, Container) inherits `Container::isContainer()`, so a single native-host detection re-ran the same env/filesystem probe up to three times. `Container::isContainer()` now memoises its result for the run (cleared on `Environment::reset()`), so the probe runs once while a subclass overriding `isContainer()` still opts out. This cut `benchDetectDrupalNative` by ~19% locally (more on Linux, where the probe actually reads `/proc/1/cgroup`). The remaining single probe is inherent to detecting containerisation. | ||
| 2. **Duplicated dispatch.** `AbstractPlatform::contextualize()` and `AbstractStack::contextualize()` were byte-identical; they now share the `DispatchesContextualization` trait, which dispatches the built-in Drupal context through its typed interface and falls back to reflection only for custom contexts. | ||
| 3. **`is()` reads the env var, by design.** `Environment::is()` calls `getenv('ENVIRONMENT_TYPE')` each time. This is deliberate: the env var is the single source of truth for the type - both the override input and the published output. Caching it in a static would create a second store that can silently diverge, so the per-call `getenv()` stays. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Keep the verdict cross-reference aligned with the renamed section.
Line 13 still points readers to “Optimization opportunities”, but this change renames the section to “Optimization notes”, so the README now refers to a heading that no longer exists.
🧰 Tools
🪛 LanguageTool
[grammar] ~32-~32: Ensure spelling is correct
Context: ...e times. Container::isContainer() now memoises its result for the run (cleared on `Env...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/README.md` around lines 28 - 34, Update the README cross-reference
in the verdict section so it matches the renamed heading; the text currently
points to “Optimization opportunities” while the section is now called
“Optimization notes.” Adjust the wording in the relevant README summary near the
optimization writeup so readers are directed to the new section title and no
stale heading name remains.
Comment on lines
+19
to
+44
| trait DispatchesContextualization { | ||
|
|
||
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function contextualize(ContextInterface $context): void { | ||
| // The built-in Drupal context is dispatched through the typed interface so | ||
| // the common path never pays for reflection; a ring that does not | ||
| // contextualize Drupal is simply a no-op here. | ||
| if ($context instanceof Drupal) { | ||
| if ($this instanceof DrupalContextualizerInterface) { | ||
| $this->contextualizeDrupal($context); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| // A custom context falls back to a contextualize<ContextName>() method | ||
| // resolved from its short name, when the ring defines one. | ||
| $method = 'contextualize' . (new \ReflectionClass($context))->getShortName(); | ||
| $callable = [$this, $method]; | ||
|
|
||
| if (is_callable($callable)) { | ||
| $callable($context); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Keep contextualization dispatch inline in the abstract bases.
This new trait centralizes logic the repository intentionally keeps duplicated in AbstractPlatform and AbstractStack; please revert the extraction unless the platform/stack contracts have actually diverged. Based on learnings, “keep the duplicated contextualization dispatch logic inline inside the abstract base classes … don’t refactor into a shared trait/common helper just to deduplicate.”
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 38-38: Missing class import via use statement (line '38', column '38'). (undefined)
(MissingImport)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/DispatchesContextualization.php` around lines 19 - 44, The
contextualization dispatch was extracted into DispatchesContextualization, but
this logic is meant to stay duplicated inline in AbstractPlatform and
AbstractStack. Revert the trait-based sharing and restore the
contextualize(ContextInterface $context) dispatch logic directly in those
abstract base classes, keeping the existing Drupal fast path and custom
contextualize<ContextName>() fallback behavior within each class rather than via
a shared helper or trait.
Source: Learnings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Every container-family stack (
Ddev,Lando,Container) inheritsContainer::isContainer(), so a single native-host detection was re-running the same env/filesystem probe - includingfile_get_contents('/proc/1/cgroup')on Linux - up to three times per call toEnvironment::init().Container::isContainer()now memoises its result in a static property thatEnvironment::reset()clears, cutting the probe from three calls to one. A subclass overridingisContainer()opts out of the cache automatically and is probed on its own terms.AbstractPlatform::contextualize()andAbstractStack::contextualize()were byte-identical; both now use a newDispatchesContextualizationtrait that dispatches the built-in Drupal context through its typedDrupalContextualizerInterfacefast path and falls back to reflection only for custom (non-Drupal) contexts.No public API or behaviour is changed.
Changes
Performance
Container::isContainer()memoises its probe result inContainer::$cachedIsContainer(aprotected static ?bool).protected detectContainer()method, whichisContainer()calls via??=on the first invocation.Container::resetCache()(newpublic staticmethod) nulls the cache;Environment::reset()calls it so test isolation is maintained.benchDetectDrupalNative(the library's primary use case); the gain is larger on Linux where the cgroup probe performs a real filesystem read.Maintainability
src/DispatchesContextualization.phptrait extracted from the two previously-identicalcontextualize()implementations.AbstractPlatformandAbstractStackeach drop their inlinecontextualize()and gainuse DispatchesContextualization;instead.#[CoversTrait(DispatchesContextualization::class)]so coverage tracking follows the moved code.Docs
benchmarks/README.mdsection renamed from "Optimization opportunities" to "Optimization notes" and updated to reflect what was done: the container probe fix, the dispatch dedup, and the intentionalgetenv()call inEnvironment::is().CI benchmark comparison
Detection cold-path
modefrom the report-only benchmark on the CI runner (Linux, PHP 8.3), comparingmain(the running trend on the "Performance benchmarks" issue) against this branch - same committed baseline, same runner class:mainbenchDetectLocalbenchDetectDrupalNativebenchDetectDrupalContainerbenchDetectPlatformbenchDetectFullStackbenchDetectDrupalNative(native host + active Drupal context) is the only path that moves beyond runner noise: it ran the container probe three times and now runs it once, so on Linux - where the probe reads/proc/1/cgroup- the cold detection nearly halves. The other subjects sit within the ~15-25% run-to-run runner variance.Before / After
Container probe - native host, three container-family stacks in the registry
Dispatch -
contextualize()on platform and stackSummary by CodeRabbit
New Features
Bug Fixes