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.

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

Eliminate some interface dispatch around object pool use #1205

Merged
merged 1 commit into from
May 23, 2023

Conversation

geeknoid
Copy link
Contributor

@geeknoid geeknoid commented May 22, 2023

  • Use concrete types for fields instead of interface types, which avoids interface dispatch and enables inlining.

The issue or feature being addressed

Details on the issue fix or feature implementation

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@geeknoid geeknoid force-pushed the geeknoid/optims branch 2 times, most recently from 9ceaf2c to 0d84132 Compare May 22, 2023 22:58
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2023

Codecov Report

Merging #1205 (3c63036) into main (71b760a) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main    #1205      +/-   ##
==========================================
- Coverage   83.31%   83.30%   -0.02%     
==========================================
  Files         277      275       -2     
  Lines        6286     6282       -4     
  Branches     1023     1023              
==========================================
- Hits         5237     5233       -4     
  Misses        844      844              
  Partials      205      205              
Flag Coverage Δ
linux ?
macos 83.30% <100.00%> (-0.02%) ⬇️
windows 83.30% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...Polly.Core/Hedging/Controller/HedgingController.cs 100.00% <ø> (ø)
src/Polly.Core/Utils/ObjectPool.cs 100.00% <ø> (ø)
...Core/Hedging/Controller/HedgingExecutionContext.cs 100.00% <100.00%> (ø)
...rc/Polly.Core/Utils/CancellationTokenSourcePool.cs 100.00% <100.00%> (ø)

@geeknoid geeknoid requested a review from martintmk May 23, 2023 04:02
@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label May 23, 2023
@martintmk martintmk added this to the v8.0.0 milestone May 23, 2023
Copy link
Contributor

@martintmk martintmk left a comment

Choose a reason for hiding this comment

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

I am wondering what's the real impact of this change.

Can you run the Polly.Core.Benchmarks (at least for timeout startegy) to see the difference?

edit: the benchmarking needed some love, so I addressed this here:
#1206

@geeknoid geeknoid changed the title Improve perf around object pools. Eliminate some interface dispatch around object pool use May 23, 2023
@geeknoid geeknoid enabled auto-merge May 23, 2023 14:23
@geeknoid
Copy link
Contributor Author

The benchmark around combining the pool types into one were inconclusive, so I just undid that part of the PR.

@geeknoid geeknoid merged commit 0e25e95 into main May 23, 2023
@geeknoid geeknoid deleted the geeknoid/optims branch May 23, 2023 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants