Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Performance optimization #387

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

Performance optimization #387

wants to merge 24 commits into from

Conversation

samdark
Copy link
Member

@samdark samdark commented Feb 9, 2025

Before:

\Yiisoft\Di\Tests\Benchmark\ContainerMethodHasBench

    benchPredefinedExisting.................R1 I2 - Mo132.604μs (±1.14%)
    benchUndefinedExisting..................R1 I2 - Mo124.260μs (±0.57%)
    benchUndefinedNonexistent...............R1 I4 - Mo455.709μs (±0.86%)

\Yiisoft\Di\Tests\Benchmark\ContainerBench

    benchConstruct..........................R1 I4 - Mo320.728μs (±1.86%)
    benchSequentialLookups # 0..............R3 I3 - Mo620.365μs (±0.44%)
    benchSequentialLookups # 1..............R3 I4 - Mo643.744μs (±0.72%)
    benchSequentialLookups # 2..............R1 I0 - Mo644.136μs (±1.16%)
    benchRandomLookups # 0..................R1 I4 - Mo623.952μs (±0.69%)
    benchRandomLookups # 1..................R1 I4 - Mo649.820μs (±1.61%)
    benchRandomLookups # 2..................R2 I4 - Mo643.426μs (±1.05%)
    benchRandomLookupsComposite # 0.........R2 I4 - Mo2.315ms (±0.71%)
    benchRandomLookupsComposite # 1.........R2 I4 - Mo2.330ms (±1.03%)
    benchRandomLookupsComposite # 2.........R1 I4 - Mo2.317ms (±1.05%)

After:

\Yiisoft\Di\Tests\Benchmark\ContainerMethodHasBench

    benchPredefinedExisting.................R5 I4 - Mo88.505μs (±1.04%)
    benchUndefinedExisting..................R2 I2 - Mo86.247μs (±1.94%)
    benchUndefinedNonexistent...............R2 I4 - Mo454.755μs (±1.42%)

\Yiisoft\Di\Tests\Benchmark\ContainerBench

    benchConstruct..........................R2 I4 - Mo219.828μs (±2.02%)
    benchSequentialLookups # 0..............R1 I0 - Mo494.586μs (±0.64%)
    benchSequentialLookups # 1..............R1 I4 - Mo541.868μs (±0.67%)
    benchSequentialLookups # 2..............R1 I4 - Mo540.820μs (±2.01%)
    benchRandomLookups # 0..................R1 I1 - Mo520.341μs (±0.51%)
    benchRandomLookups # 1..................R1 I4 - Mo541.799μs (±1.23%)
    benchRandomLookups # 2..................R1 I0 - Mo547.495μs (±1.06%)
    benchRandomLookupsComposite # 0.........R1 I4 - Mo2.215ms (±1.22%)
    benchRandomLookupsComposite # 1.........R1 I1 - Mo2.170ms (±1.19%)
    benchRandomLookupsComposite # 2.........R1 I2 - Mo2.174ms (±0.65%)

samdark and others added 7 commits February 9, 2025 23:22
@samdark
Optimize Container::has() method performance by checking definitions …
3df3fb7 
…first

- Reorder checks to prioritize definition lookup which is the most common case
- Return true immediately if definition exists
- Only check tag aliases if definition doesn't exist
- Return false if neither exists

Performance improvements:
- benchPredefinedExisting: ~29% faster (130μs → 92μs)
- benchUndefinedExisting: ~29% faster (123μs → 87μs)
- benchUndefinedNonexistent: ~1% faster (446μs → 441μs)
- Small improvements in other benchmarks
@samdark
Optimize Container::get() method performance
f18c1c6 
- Add fast path for existing instances
- Extract StateResetter logic into separate method
- Move StateResetter check to the end to avoid duplicate instance access
- Improve code readability and maintainability

Performance improvements:
- benchSequentialLookups: ~3% faster (620μs → 599μs)
- benchUndefinedExisting: ~5% faster (87μs → 83μs)
- Other benchmarks show similar or slightly improved performance
@samdark
Optimize Container::validateDefinition() method performance
f97bd53 
- Add early returns for common simple cases (strings, ContainerInterface, Closure)
- Simplify array definition handling
- Improve code readability and reduce nesting

Performance improvements:
- benchConstruct: ~29% faster (336μs → 238μs)
- benchSequentialLookups: ~13% faster (599μs → 523μs)
- benchRandomLookups: ~16% faster (621μs → 524μs)
- benchRandomLookupsComposite: ~9% faster (2.3ms → 2.1ms)
- benchUndefinedNonexistent: ~5% faster (464μs → 440μs)
@samdark
Optimize Container::build() method performance
3c367d3 
- Add normalized definitions cache with memory management
- Reorder checks to prioritize most common cases
- Add fast path for circular reference check
- Improve code readability with better comments

Performance improvements:
- benchRandomLookupsComposite: ~4% faster (2.3ms → 2.2ms)
- benchSequentialLookups: ~2% faster (544μs → 539μs)
- benchRandomLookups: ~1.5% faster (546μs → 525μs)

Memory usage is kept in check by clearing the cache when it reaches 100 entries.
@samdark
Optimize Container::get() method performance
ca8064e 
- Simplify exception handling paths
- Add fast path for NotFoundException with matching ID
- Use ternary operators for cleaner code
- Improve code readability with better comments

Performance improvements:
- benchRandomLookupsComposite: ~1% faster (2.2ms → 2.18ms)
- benchSequentialLookups: ~0.5% faster (539μs → 536μs)
- benchRandomLookups: ~0.5% faster (541μs → 538μs)
@samdark
Optimize Container::addDefinition() method performance
bf3fadc 
- Skip parsing if already a valid definition
- Only validate meta if it's not empty
- Only unset instance if it exists
- Improve code readability with better comments

Performance improvements:
- benchConstruct: ~10% faster (238μs → 213μs)
- benchSequentialLookups: ~3.5% faster (536μs → 517μs)
- benchRandomLookups: ~3.7% faster (538μs → 518μs)
- benchRandomLookupsComposite: ~0.5% faster (2.18ms → 2.17ms)
@StyleCIBot
Apply fixes from StyleCI
2f99923
@codecov Codecov
Copy link

codecov bot commented Feb 9, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.39326% with 13 lines in your changes missing coverage. Please review.

Project coverage is 97.15%. Comparing base (b76e4bb) to head (696b19f).

Files with missing lines Patch % Lines
src/CompositeContainer.php 25.00% 12 Missing ⚠️
src/Container.php 98.63% 1 Missing ⚠️
Additional details and impacted files 
@@              Coverage Diff              @@
##              master     #387      +/-   ##
=============================================
- Coverage     100.00%   97.15%   -2.85%     
- Complexity       169      184      +15     
=============================================
  Files             11       11              
  Lines            492      528      +36     
=============================================
+ Hits             492      513      +21     
- Misses             0       15      +15

☔ 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.

@samdark samdark requested a review from a team February 9, 2025 22:09
xepozz
xepozz requested changes Feb 16, 2025
View reviewed changes
vjik
vjik requested changes Feb 16, 2025
View reviewed changes
@samdark samdark changed the title Perormance optimization Performance optimization Feb 17, 2025
@samdark
Copy link
Member Author

samdark commented Mar 4, 2025

Did some fixes. @vjik can you help with Psalm? I can ignore mixed returns but, I guess, it could be solved in a better way.

@samdark samdark requested review from vjik and xepozz March 4, 2025 23:02
@vjik
Copy link
Member

vjik commented Mar 16, 2025

Did some fixes. @vjik can you help with Psalm? I can ignore mixed returns but, I guess, it could be solved in a better way.

Fixed. I also suppresed MixedReturnStatement because mixed return type is OK for get() method.

@samdark
Copy link
Member Author

samdark commented Mar 20, 2025

@vjik test merged and is now passing with latest commit.

vjik
vjik requested changes Mar 22, 2025
View reviewed changes
@samdark
Copy link
Member Author

samdark commented Mar 26, 2025

All comments solved. @vjik anything else?

@samdark samdark requested a review from vjik March 26, 2025 14:14
@vjik
Copy link
Member

vjik commented Mar 26, 2025 •
edited
Loading

All comments solved. @vjik anything else?

Looks good. It remains to add the tests. Test coverage is reduced.

See: https://app.codecov.io/gh/yiisoft/di/pull/387

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

@xepozz xepozz Awaiting requested review from xepozz

@vjik vjik Awaiting requested review from vjik

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samdark @vjik @xepozz @StyleCIBot