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

Further possible fix to reduce memory usage #6390

Merged
merged 4 commits into from
Apr 9, 2019
Merged

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Mar 28, 2019

@TIHan and others have noticed excessive creation of ILModuleReader. Here's a second possible fix, to be evaluated

The first possible fix is at #6389

@dsyme
Copy link
Contributor Author

dsyme commented Apr 9, 2019

@KevinRansom I prefer this one to #6389

@TIHan What do you think?

@davidkean may also want to review

@KevinRansom
Copy link
Member

Thanks

@KevinRansom KevinRansom merged commit c370f8e into dotnet:master Apr 9, 2019
brettfo added a commit that referenced this pull request Apr 9, 2019
brettfo added a commit that referenced this pull request Apr 9, 2019
@brettfo
Copy link
Member

brettfo commented Apr 9, 2019

FYI, I just had to revert this in #6468 due to a build failure in master (link in revert PR).

KevinRansom added a commit that referenced this pull request Apr 9, 2019
KevinRansom added a commit that referenced this pull request Apr 9, 2019
KevinRansom added a commit that referenced this pull request Apr 9, 2019
@KevinRansom
Copy link
Member

KevinRansom commented Apr 9, 2019 via email

@dsyme
Copy link
Contributor Author

dsyme commented Apr 10, 2019

@auduchinok asked about this. The use of two caches is explained by these two comments:

// Cache to extend the lifetime of a limited number of readers that are otherwise eligible for GC
...
// Cache to reuse readers that have already been created and are not yet GC'd

@cartermp cartermp mentioned this pull request Apr 19, 2019
8 tasks
@TIHan
Copy link
Contributor

TIHan commented May 25, 2019

I have evaluated this change and with the current internal project that I've been testing, this significantly reduced memory consumption after a find all refs. Before, we would reach 2.6gb in VS process, and now it hits 1.9gb specifically with this change.

This is fantastic.

I was wondering why 16.1 was not hitting the 2.6gb limit.

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.

4 participants