-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 project string cache under a change wave. #7965
Eliminate project string cache under a change wave. #7965
Conversation
Would it be cleaner to put this check in the cache rather than all its callers? |
Ah, yes, nice idea, I will move the check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor:
Can we also add an early out for Clear? No sense taking a lock when we know it's empty.
About avoiding taking the lock. I wonder what happens if somebody would mix calls with and without disabling the change wave 17.4. It then may lead to errors and memory leaks. If we should care about that case, I would prefer to keep it as it is. I do not expect Clear to be called too often, since the capacity of strong ProjectRootElementCache is bigger now and there are not so many rotations in it, which triggers the clear. |
I think it would be ok. First, I think most people set it via environment variable, so you'll often either run builds with it enabled or disabled but not mixed. Second, getting an early out for clear would mean the disabled version would just not touch the cache the whole time, and the enabled version would touch then eventually clear the cache by itself, so they shouldn't interfere with each other. I don't think it matters too much, though. |
Co-authored-by: Forgind <Forgind@users.noreply.github.com>
I changed the ChangeWave version in this PR from 17.4 to 17.6, since this PR did not get into 17.4. |
Fixes #5444
Context
The
ProjectStringCache
does seem to have a memory leak problem when used from VS.The reason of the leak is that it clears memory on the event from
ProjectRootElementCache
, which is raised when project is moved out of the strong cache. The unproper use ofProjectRootElementCache
might lead toProjectStringCache
not freeing memory.Also, there were doubts if this cache adds anything at all to performance. Experiments does not show significant difference for the two cases.
We decided to remove
ProjectStringCache
under a change wave.Changes Made
ProjectStringCache
under change waves below 17.6.Testing
Unit tests
Experimental insertion
Notes
ProjectStringCache
altogether with the related code): Eliminate project string cache #7952.StringTools
instead.