Skip to content

Performance optimizations#400

Open
samdark wants to merge 18 commits into
masterfrom
performance
Open

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

Conversation

@samdark

@samdark samdark commented Apr 23, 2026

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

codecov Bot commented Apr 23, 2026

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 3 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 Sentry.
📢 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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants