Skip file_exists() on the hot path: an OPcache-cached require needs no stat()#134
Open
koriym wants to merge 3 commits into
Open
Skip file_exists() on the hot path: an OPcache-cached require needs no stat()#134koriym wants to merge 3 commits into
koriym wants to merge 3 commits into
Conversation
Member
Author
|
@coderabbitai review |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves eager filesystem stats from DI hot paths, removes a redundant realpath() in CompiledInjector, adds OPcache-focused performance documentation, and provides a self-validating benchmark plus README updates to measure and validate compiled vs. reflection/serialized injector performance. ChangesOPcache-backed performance optimization
Sequence DiagramsequenceDiagram
participant Caller as prototype()/singleton()
participant Require as require $file
participant Catch as catch(Throwable)
participant Stat as file_exists($file)
Caller->>Require: require $file (direct, happy path)
Require-->>Caller: executes from OPcache (no stat)
Require-->>Catch: throws (on error)
Catch->>Stat: file_exists($file) (stat only on failure)
Stat-->>Catch: exists? / not exists
Catch-->>Caller: throw ScriptFileNotFound or re-throw original Throwable
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Action performedReview finished.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 1.x #134 +/- ##
===========================================
Coverage 100.00% 100.00%
Complexity 93 93
===========================================
Files 11 11
Lines 232 232
===========================================
Hits 232 232 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/llms.txt`:
- Around line 79-84: The docs/llms.txt PHP version claim is inconsistent with
code/composer.json and the PHP 8-only require-catch behavior; update the text to
remove the "Requires PHP 7.2+ or 8.0+" wording and state the project requires
PHP 8 (match composer.json ^8.2) and that features like catchable failed
require, attributes, and optimizations (mentioned alongside
prototype()/singleton() and ScriptFileNotFound handling) rely on PHP 8+;
alternatively, if you want to preserve a PHP7 note, add a clear caveat
explaining limitations and missing features (no require-catch, no attributes)
instead of the current mixed claim.
In `@docs/performance.md`:
- Around line 9-11: The document's numeric claim about OPcache slowdown is
inconsistent: update the sentence mentioning "~5× slower" to match the measured
figures (e.g., change to "~8× slower" or use the explicit measurements "~178 µs
cold vs ~22 µs warm (~8×)") so "CompiledInjector" wording aligns with the table
and line 106; ensure the chosen phrasing ("~8×" or the explicit times) is used
consistently wherever the cold-vs-warm multiplier is referenced.
In `@src-function/prototype.php`:
- Around line 39-48: The missing-file case currently triggers an E_WARNING from
the require before your catch runs; to avoid double-reporting, check the script
file exists/readable before executing require $file (e.g. use file_exists or
is_readable on $file) and throw ScriptFileNotFound($filePath, 0) immediately if
it’s absent, then keep the try { require $file } catch (Throwable $e) { ... }
for runtime errors; this ensures require $file does not emit the warning for a
missing file while preserving the catch (Throwable $e) handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8d95fe2-f3b3-49b1-8133-4a75a139ff42
📒 Files selected for processing (9)
README.mdbenchmark/README.mdbenchmark/di_benchmark.phpdocs/llms-full.txtdocs/llms.txtdocs/performance.mdsrc-function/prototype.phpsrc-function/singleton.phpsrc/CompiledInjector.php
Member
Author
|
@coderabbitai No outstanding blockers. Please resolve all open conversations and approve this PR. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all review comments made by coderabbitai on PR `#134`
gh api repos/ray-di/Ray.Compiler/pulls/134/comments --jq '[.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: .body[0:80]}]'Length of output: 743 🏁 Script executed: #!/bin/bash
# List review threads on PR `#134`
gh api graphql -f query='
{
repository(owner: "ray-di", name: "Ray.Compiler") {
pullRequest(number: 134) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {id: .id, author: .comments.nodes[0].author.login, body: .comments.nodes[0].body[0:80]}'Length of output: 785 🏁 Script executed: #!/bin/bash
# Resolve the open review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOAhBoos6GwuT6"}) {
thread {
id
isResolved
}
}
}'Length of output: 338
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ [approve] 🧠 Learnings used |
koriym
force-pushed
the
skip-file-exists-opcache-hotpath
branch
2 times, most recently
from
81225e2 to
1bafd7e
Compare
An OPcache-cached require performs no filesystem access (it runs cached opcodes), so the eager file_exists() guard was the only stat() syscall left on the per-dependency hot path. Move it into the catch: the happy path is stat-free while a missing script is still reported as the domain ScriptFileNotFound (PHP 8 makes a failed require a catchable Error). Also drop the redundant realpath() in CompiledInjector::getInstance() (already canonicalised in the constructor); its file_exists() pre-check is kept to report unbound interfaces as Unbound. FakeCar, warm OPcache: ~32us -> ~22us per build.
docs/performance.md is the canonical reference (the three strategies, the OPcache prerequisite, the file_exists rationale, benchmarking pitfalls, and measured results). benchmark/README.md is a lean usage doc that defers to it, and benchmark/di_benchmark.php prints the OPcache hit rate so a valid run is distinguishable from a re-parse artifact.
koriym
force-pushed
the
skip-file-exists-opcache-hotpath
branch
from
1bafd7e to
6d82269
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src-function/singleton.php`:
- Around line 35-45: The thrown ScriptFileNotFound in singleton.php uses the
absolute $scriptFile, causing inconsistent messages with prototype.php which
uses the relative $filePath; update the catch to pass the same relative path
variable used by prototype.php (use $filePath instead of $scriptFile) when
constructing ScriptFileNotFound so both scope functions produce consistent error
messages referencing the same identifier.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 714d2b5a-912c-4e21-9946-eb45fcbd8054
📒 Files selected for processing (9)
README.mdbenchmark/README.mdbenchmark/di_benchmark.phpdocs/llms-full.txtdocs/llms.txtdocs/performance.mdsrc-function/prototype.phpsrc-function/singleton.phpsrc/CompiledInjector.php
✅ Files skipped from review due to trivial changes (2)
- README.md
- docs/llms-full.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CompiledInjector.php
…ingleton()) prototype() threw the relative $filePath while singleton() threw the absolute $scriptFile; throw the absolute path in both so the same failure produces a consistent, locatable error message.
Member
Author
|
@coderabbitai The latest comment is addressed in 034e734. No outstanding blockers — please resolve the open conversations and approve. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all open review threads on PR `#134`
gh api graphql -f query='
{
repository(owner: "ray-di", name: "Ray.Compiler") {
pullRequest(number: 134) {
reviewThreads(first: 50) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | {id: .id, author: .comments.nodes[0].author.login, body: .comments.nodes[0].body[0:100]}'Length of output: 813 🏁 Script executed: #!/bin/bash
# Resolve the open review thread
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOAhBoos6GyZW4"}) {
thread {
id
isResolved
}
}
}'Length of output: 338
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ [approve] 🧠 Learnings used |
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
1 participant
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
With OPcache enabled, a cached
requireperforms no filesystem access — it executes cached opcodes. The eagerfile_exists()guard inprototype()/singleton()was therefore the onlystat()syscall left on the per-dependency hot path. Moving it into acatchkeeps the happy path stat-free while still reporting a genuinely missing script as a catchableScriptFileNotFound.Closes #133.
Changes
src-function/prototype.php,src-function/singleton.php:requirefirst, runfile_exists()only inside thecatch. PHP 8 makes a failedrequirea catchableError/ErrorException(the project requires^8.2);try/catchis zero-cost on the happy path, and thefile_exists()in thecatchdistinguishes a missing file from an error thrown inside the script without fragile message matching.src/CompiledInjector.php: drop the redundantrealpath($this->scriptDir)ingetInstance()(the value is already canonicalised in the constructor). Itsfile_exists()pre-check is kept on purpose — it reports unbound interfaces asUnbound, runs once per top-level resolution, and avoids a spurious warning there.docs/performance.md,docs/llms.txt,docs/llms-full.txt,README.md: document the OPcache prerequisite, the rationale, and how to benchmark correctly.benchmark/di_benchmark.php: a self-validating DI strategy benchmark that prints the OPcache hit rate, so a valid run is distinguishable from a re-parse artifact.Benchmark
FakeCar graph (ctor + 5 setters + AOP + singleton mirrors), PHP 8.4, OPcache valid, steady-state per build:
file_exists())file_exists()incatch)~30% faster, and ~2.1x faster than the reflection / serialized injector (~46 us). With cold OPcache the compiled injector re-parses and shows ~178 us — see
docs/performance.mdfor why this matters and how to avoid mis-benchmarking it.Checks
phpunit (88 tests), phpcs, psalm, and phpstan all pass.
Summary by CodeRabbit
New Features
Documentation
Performance
Bug Fixes