Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Performance optimizations#400

Open
samdark wants to merge 18 commits into
master from
performance
Open

Performance optimizations #400
samdark wants to merge 18 commits into
master from
performance

Conversation

@samdark

@samdark samdark commented Apr 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member
Q A
Is bugfix? ❌
New feature? ❌
Breaks BC? ❌
Fixed issues -

Performance optimizations. Overall results compared to master:

Benchmark Set master baseline performance Change
ContainerMethodHasBench::benchPredefinedExisting - 12.944μs 9.339μs -27.85%
ContainerMethodHasBench::benchUndefinedExisting - 8.705μs 4.888μs -43.84%
ContainerMethodHasBench::benchUndefinedNonexistent - 76.135μs 4.806μs -93.69%
ContainerBench::benchConstruct - 46.537μs 24.197μs -48.01%
ContainerBench::benchSequentialLookups 0 97.115μs 71.211μs -26.67%
ContainerBench::benchSequentialLookups 1 101.089μs 75.627μs -25.19%
ContainerBench::benchSequentialLookups 2 103.364μs 77.133μs -25.38%
ContainerBench::benchRandomLookups 0 99.784μs 71.519μs -28.33%
ContainerBench::benchRandomLookups 1 103.061μs 76.196μs -26.07%
ContainerBench::benchRandomLookups 2 102.671μs 77.168μs -24.84%
ContainerBench::benchRandomLookupsComposite 0 388.613μs 44.701μs -88.50%
ContainerBench::benchRandomLookupsComposite 1 393.125μs 45.335μs -88.47%
ContainerBench::benchRandomLookupsComposite 2 393.340μs 45.489μs -88.44%

What was done:

  • f9b88a6: Cache Container::has() results and clear them when definitions change.
  • f1e6568: Fast-return from validation for valid string definitions.
  • 63e5e2c: Avoid duplicate has() before get() in build().
  • a9818c3: Skip DefinitionParser::parse() for non-array definitions.
  • 10ec99d: Inline bulk addDefinitions() logic to reduce per-definition call overhead.
  • c6dcb5c: Batch-prepare initial definitions before constructing DefinitionStorage.
  • a77213d: Return early from delegate setup when no delegates are configured.
  • 423766a: Split definition preparation into validated and no-validation branches.

There's a BC break because it makes a cache cap for has() configurable by adding it to the interface.

Copilot AI review requested due to automatic review settings April 23, 2026 23:22

codecov Bot commented Apr 23, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (59579ea) to head (cc81a0a).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files 
@@ Coverage Diff @@
## master #400 +/- ##
===========================================
 Coverage 100.00% 100.00% 
- Complexity 184 221 +37 
===========================================
 Files 11 11 
 Lines 500 599 +99 
===========================================
+ Hits 500 599 +99

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Performance-focused update to the DI container implementation, adding internal caching and restructuring definition preparation to reduce overhead in common code paths.

Changes:

  • Added caching for Container::has() results and CompositeContainer delegate lookups.
  • Reworked definition preparation/validation paths (including a no-validation fast path) and reduced redundant work in build().
  • Expanded benchmarks and added a unit test for empty-string class definitions.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/Container.php Adds has() caching, refactors definition preparation/validation, and optimizes build path.
src/CompositeContainer.php Adds lookup caching for faster delegate resolution.
tests/Unit/ContainerTest.php Adds regression test for empty string definition validation.
tests/Benchmark/ContainerBench.php Adds no-validation benchmark variants for construct/lookups.
tests/Benchmark/TypicalUsageBench.php Introduces a new benchmark suite covering typical container usage scenarios.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/CompositeContainer.php Outdated
Comment thread src/Container.php
Comment thread src/Container.php
Comment thread src/Container.php
Comment thread src/CompositeContainer.php
@samdark samdark requested a review from Copilot April 23, 2026 23:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Container.php
@samdark samdark requested a review from Copilot April 23, 2026 23:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Container.php Outdated 
� Conflicts:
�	src/Container.php
@samdark samdark requested a review from Copilot April 24, 2026 00:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 29 to +36
*/
public function shouldValidate(): bool;

/**
* @return int Maximum number of cached `has()` results. `0` disables the cache.
*/
public function getHasCacheLimit(): int;

Copilot AI Apr 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding getHasCacheLimit() to ContainerConfigInterface is a backwards-incompatible API change: any downstream code that provides its own ContainerConfigInterface implementation will now fail to compile until it adds this method. The PR metadata says "Breaks BC? ❌"; either adjust that statement / release notes accordingly, or avoid changing the existing interface (e.g., add a separate optional interface and fall back to the default limit when the method is absent).

Copilot uses AI. Check for mistakes.

samdark commented Apr 24, 2026

Copy link
Copy Markdown
Member Author

Should be re-evaluated after yiisoft/definitions#114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle によって変換されたページ (->オリジナル) /