Skip to content

Conversation

yuehuang010
Copy link
Contributor

Remove duplicated strings from Item and Property Function. Intern these strings to reduce memory usage.

Context

Strings used parsing Item and Property Function are duplicated many times, common offenders are "Contains", "Strings", and "true".

Changes Made

Use the Strings.WeakIntern() tool to cache the strings. Since these are common strings which are shared among projects, use the global String store instead of the project specific store.

Testing

Profiled with the Visual Studio Memory Inspector. This reduced 5-6mb of strings saved in our internal Repro of 200 projects.

Notes

There are still other places here duplicate string exists, this PR focus only on these locations.

@AR-May AR-May requested review from ladipro and maridematte July 18, 2023 13:40
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

This reduced 5-6mb of strings saved in our internal Repro of 200 projects.

Out of curiosity, how much is it in relative terms - i.e. compared to the total amount allocated when evaluating the projects?

Also, since these are hot code paths, have you confirmed that the change doesn't regress evaluation throughput / CPU time?

@yuehuang010
Copy link
Contributor Author

yuehuang010 commented Jul 18, 2023

Out of curiosity, how much is it in relative terms - i.e. compared to the total amount allocated when evaluating the projects?

When all projects are loaded, it took 700-1000mb, depending on GC. Thus, the saving is small but measurable.

Also, since these are hot code paths, have you confirmed that the change doesn't regress evaluation throughput / CPU time?

I haven't noticed any major perf regression, but this change would only show up in CPU micro-benchmarks. I have used the memory profiler to verify that the strings are no longer allocated after the change.

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