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

Allocate WeakKeyDictionary`2 with smaller initial capacities #222

Merged
merged 1 commit into from
Apr 4, 2018
Merged

Allocate WeakKeyDictionary`2 with smaller initial capacities #222

merged 1 commit into from
Apr 4, 2018

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Feb 27, 2018

Fixes #221

@AArnott AArnott added the perf label Feb 27, 2018
@AArnott AArnott added this to the v15.7 milestone Feb 27, 2018
@AArnott AArnott requested a review from lifengl February 27, 2018 07:21
@AArnott
Copy link
Member Author

AArnott commented Feb 27, 2018

@lifengl Can you verify this change has the impact you expect?

lifengl
lifengl approved these changes Apr 3, 2018
View reviewed changes
Copy link
Member

@lifengl lifengl left a comment

Choose a reason for hiding this comment

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

The change looks good. I finally got a chance to verify it with loading Roslyn. The number of live tasks can change a lot, so only take memory snapshots after the product cool down gives some reasonable data. The result shows the memory usage of those collection is reduced about half with this change.

@lifengl
Copy link
Member

lifengl commented Apr 3, 2018

Sorry for the delay. I got two dumps with/without this change when I use VS to open Roslyn solution. Both taken when the product is cool down, and forced GC. The number of collections are similar:
With change 148,393 collections, and without it 148,453 collections (it is unrelated to the change, just to make sure that the two dumps are captured with relatively similar condition.

Without this change, those collections use 12M memory, and with it, they use 6M. This change is about to reduce the usage in half.

@lifengl lifengl closed this Apr 3, 2018
@lifengl
Copy link
Member

lifengl commented Apr 3, 2018

sorry, click the close and comment button by a mistake

@lifengl lifengl reopened this Apr 3, 2018
@AArnott
Copy link
Member Author

AArnott commented Apr 3, 2018

Great. I can merge this PR for master now, but do you want to take it for 15.7? If so, can you let me know when the ask mode approval is obtained?

@lifengl
Copy link
Member

lifengl commented Apr 4, 2018

Thinking about the overhead to doing it through ship room, especially no update for the threading library in 15.7, I will leave it to 15.8.

@AArnott AArnott changed the base branch from v15.7 to master April 4, 2018 23:53
@AArnott AArnott merged commit 022a526 into master Apr 4, 2018
@AArnott AArnott deleted the dev/andarno/fix221 branch April 4, 2018 23:53
AArnott pushed a commit to AArnott/vs-threading that referenced this pull request Oct 24, 2023
@dependabot
Bump dotnet-coverage from 17.8.6 to 17.9.1 (microsoft#222)
7df6655 
Bumps [dotnet-coverage](https://github.com/microsoft/codecoverage) from 17.8.6 to 17.9.1.
- [Commits](https://github.com/microsoft/codecoverage/commits)

---
updated-dependencies:
- dependency-name: dotnet-coverage
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@AArnott AArnott mentioned this pull request Oct 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lifengl lifengl lifengl approved these changes

Assignees

@AArnott AArnott

@lifengl lifengl

Labels
perf
Projects
None yet
Milestone
v15.7
Development

Successfully merging this pull request may close these issues.
2 participants
@AArnott @lifengl