Update the WeakStringCache to keep strong references to smaller strings.

[Erarndt]

Fixes #

Context

The current string cache implementation uses weak GC handles to avoid holding onto references to strings for longer than needed. As a result, the strings are frequently collected and recreated. Additionally, weak references add additional cost to each garbage collection. The proposed implementation would keep only use a weak reference for larger strings since the cost of keeping around smaller strings generally outweighs the cost of recreating them.

Changes Made

Testing

Notes

[SimaTian]

Hello and thank you for looking into possible performance improvements.
Do you have any benchmarking around the possible performance gain please?
If yes, could you share your data?
How was the 500 char threshold selected? (I think that 500 is a threshold that will capture most of the strings that MSBuild generates)

[JanKrivanek]

Related to #11160

[Erarndt]

Hello and thank you for looking into possible performance improvements.
Do you have any benchmarking around the possible performance gain please?
If yes, could you share your data?
How was the 500 char threshold selected? (I think that 500 is a threshold that will capture most of the strings that MSBuild generates)

I have lots of various data including traces with PerfView and build times. If you're looking for something specific, I likely have it :). The value was chosen somewhat arbitrability, but I did see a good number of strings that were significantly larger (1000+). There's likely some tuning we could do on the specific value.

[JanKrivanek]

Experimental VS insertion: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/603315?_a=overview

[SimaTian]

I've updated the WeakStringCache_Tests since these were testing the cache forgetting behavior on old strings.
Some new ones for the new portion are most likely a good idea as well. I will start with the benchmarking and if it turns out good, I will proceed with the test update.
One that note @JanKrivanek, the insertion has probably failed due to this. Can you re-run the pipeline please?

[SimaTian]

I've done some basic local benchmarking of the cache.
For .net9 it has a small but visible positive impact.
For net472 the variance is too large and the impact looks to be either negligible or slightly negative. I'm unsure about the reason, also I've had fairly varied results so I wouldn't call it conclusive. Since this is weird, I will try to figure out some more stable way to run this.

main-core	cache-core	main-framework	cache-framework	cache-framework2
38.17889	38.87665	37.40668	38.99776	36.62044
38.53512	36.817	36.98474	37.37747	36.8773
35.25003	33.84592	35.36707	37.29134	38.42806
34.52955	33.32155	38.48902	39.59925	37.04002
34.46097	34.94563	35.8827	37.50032	38.92489
35.72239	33.3015	38.23811	37.06755	36.16032
37.01463	33.48803	36.5214	36.98713	36.38341
34.16286	33.07242	36.71748	37.79424	35.8911
33.84519	35.96277	39.23669	38.91536	33.90686
33.76678	33.33597	36.81749	37.10196	38.63083
33.92001	32.98285	36.23294	36.72502	36.62178
33.9255	33.34761	36.75168	36.45474	37.63606
33.7753	32.46068	36.70211	39.35933	37.43448
34.06473	35.40557	36.40363		38.14316
35.26259	34.12788	37.49066		38.88766
35.40107		37.38926		37.58297
36.33105		36.87269		37.5796
33.8218
35.10936	34.3528	37.02967	37.78242	37.17243

As a note, I'm still uncertain how to handle the spikes and what to consider a spike. For now I've removed all 40s+ values from the measurement for -core and 42s+ from the framework. I'm aware this isn't perfect so take this just as an initial estimate.
I will set up some better measurement and I also want to see VS insertion and performance test results.

[SimaTian]

Overall I like it, with some caveats.

• so far my benchmarking was inconclusive. I will have to benchmark more.
• isn't this a potential memory leak for a long-running VS sessions? (e.g. if we introduce permanent cache and some build repeatedly generates random sub-500 character strings)

[SimaTian]

More benchmarking. On a devbox it definitely helps
net472 average times after 20-ish warm builds, after removing outliers.
cache: 91.81 vs main: 93.47
Devbox is kind of slow even with dev drive.

[SimaTian]

One question that came to my mind:
how much do we care about this technically being a memory leak?

• each string that is deserialized by InterningBinaryReader get's added to this cache
• so we keep every log string that ever gets passed to us forever.
  • there are lot of duplicates, that will only get saved once, which is nice
  • however I can imagine some log string like "save this timestamp : XXX" that will stay around together to be a potential issue
  • this might be come more pronounced if MSBuild sticks around for longer - e.g. during the developer inner loop

I'm unsure about the severity of this - there is definitely a room for maneouvering and trade offs.
That being said, for example OrchardCore build on two cores generates ~100MB worth of strings(many of which will be duplicate). On more cores, there will probably be additional strings generated.

[JanProvaznik]

The code is ok, I agree it does what it claims to do.

I am concerned about the strategy with @SimaTian 's analysis.
It would speed up the average build (empirically and from theory), but in the persistent scenario (VS, core MSBuild server) the "memory leak" is bad. Customers would be quite sad when their long running VS consumed gigabytes of RAM on ever-growing MSBuild cache. The strong references would have to be purged at some time/capped.