Simplifying internal structure and logic for the SyntaxNodeCache by CyrusNajmabadi · Pull Request #80825 · dotnet/roslyn

CyrusNajmabadi · 2025-10-20T22:05:15Z

Found during an investigation with razor for similar code they had copied from roslyn. A few things were changed here.

I recommend reviewing one commit at a time.

First, the core data stored in the cache array, changed from a large struct to a single pointer. This struct could tear, which could lead to overly conservative behavior. For example, if a read from the cache overlapped a write from some otehr thread, the read could potentially see a mismatched hashcode, leading it to not accept a node from the cache it should accept.

Second: code specific to the cache was placed in GreenNode itself, despite never being used anywhere byt from the cache. This code was moved to the cache allowing everything to be self contained and not exposed to unnecessary parts of the codebase.

Third: Docs were beefed up to explain the semantics here, and why it's ok that this type has safe hashing semantics, even with a "last collision wins" implementation.

--

Data backing this up. I tested caching on all the C# files of roslyn. Here were the stats:

Number of files parsed: 15955

Number of times we checked the cached for actually cacheable items: 17,151,515
Number of cache successes: 9,536,254. Around 56%. So 56% of the time we attempt to lookup something in the cache, we can find and reuse an item.

Cache collisions: 7,539,649. Number of times we looked up in the cache, found an item, but couldn't reuse it because it didn't match what we were looking up.

Note: 7,539,649 + 9,536,254 != 17,151,515. It's about 75k short. That means only in 75k cases (0.4% of lookups) did we lookup in the cache and find nothing. Given that hte cache only has 65k elements. That means that extremely quickly it filled up entirely, and we're basically always either finding a match or hitting a collision.

Cache collisions with same kind: 37,335 (0.2% of cases). This is the number of times we collided with something existing and it had the kind. This shows that we really don't need to compare hashes with what is currently stored there as practically always the kind check is enough. Conceptually, this makes sense. Nodes with different kinds are already going to distribute themselves broadly across the entire space of the array (there are only really 500 syntax kinds, and 161 of those kinds actually have a slotcount <= 3), while the array has 65k elements. So chance of collision on diff kind is already low, as those 168 kinds will distribute well across the keyspace. Similarly, flags will generally be very similar. So they won't move things around. So to collide on a different kind, you'd need to somehow have it + the child hashes work together to give you that bad luck.

So keeping the hash around doesn't really save anything. While making the code more difficult to reason about.

--

Note: i tested parsing time before/after this with roslyn. Parsing time after the change for all those files was 1.3680783 and before 1.3965380s on average. This is likely within the margin of error.

--

Note: ideally we can keep this code entirely in sync with Razor as well. Making it easier to reason about both systems.

CyrusNajmabadi · 2025-10-21T09:27:58Z

src/Compilers/Core/Portable/Syntax/GreenNode.cs


        #endregion

-        #region Caching


this code was moved. with no changes.

src/Compilers/Core/Portable/Syntax/GreenNode.cs

src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCache.cs

…che.cs

CyrusNajmabadi · 2025-10-21T09:35:53Z

src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCache.cs

-                this.hash = hash;
-                this.node = node;
-            }
-        }


This type was not helpful. First, it made it so that one had to reason about the semantics of torn reads/writes in the array below. The semantics were correct, but were very subtle and difficult to prove. Second, extra data stored here did not help at all. The hash that was read could not actually be asserted to be anything (since collisions overwrite). And the hash could only be used, at best, to no effect to compare against the existing hash into that location. But at worse, it could disallow reuse of a node that could be reused during a tear.

The hash itself is computed from data that is already checked in IsNodeEquivalent. While technically this might reduce the number of checks performed when there was a collision, practically, this turns out to almost always be one check max as the Kind validation almost always immediately fails. See PR op for more data on this.

Note: the Kind/Flags checks are non-virtual and are just plucking raw data out of the green node. So we're not adding any indirections or anything like that.

…into removeHash

CyrusNajmabadi · 2025-10-21T10:24:28Z

@dotnet/roslyn-compiler for consideration. Found during razor code investigations.

CyrusNajmabadi · 2025-11-11T14:35:31Z

src/Compilers/CSharp/Test/Syntax/Syntax/SyntaxNodeCacheTests.cs

        {
            var child0 = new SyntaxTokenWithTrivia(SyntaxKind.IntKeyword, null, null);
-            SyntaxNodeCache.AddNode(child0, child0.GetCacheHash());
+            SyntaxNodeCache.AddNode(child0, SyntaxNodeCache.GetCacheHash(child0));


moved everything related to node caching to the SyntaxNodeCache type itself.

CyrusNajmabadi · 2025-11-30T18:26:00Z

@dotnet/roslyn-compiler this is ready for review.

CyrusNajmabadi · 2025-11-30T18:26:05Z

@ToddGrun did you make this change with Razor? If so, can you link to your PR for that?

ToddGrun · 2025-11-30T18:35:23Z

@ToddGrun did you make this change with Razor? If so, can you link to your PR for that?

dotnet/razor#12370

CyrusNajmabadi · 2026-01-15T16:38:29Z

@dotnet/roslyn-compiler this is ready for review.

CyrusNajmabadi · 2026-01-20T23:05:41Z

@dotnet/roslyn-compiler ptal.

CyrusNajmabadi · 2026-01-23T12:21:44Z

@dotnet/roslyn-compiler ptal.

src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCache.cs

…che.cs Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

…into removeHash

CyrusNajmabadi · 2026-01-23T13:47:34Z

@jjonescz can you do a /pr-val for me?

github-actions · 2026-01-23T13:59:08Z

View PR Validation Run triggered by @jjonescz

Parameters

Validation Type: pr-val
Pipeline ID: 8972
Pipeline Version: main
PR Number: 80825
Commit SHA: 30a6a9fa650c42c36644e695cab553248b99a6a9
Source Branch: removeHash
Target Branch: main
Build ID: 13155424

https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/702563

CyrusNajmabadi · 2026-01-27T20:26:47Z

@jjonescz how did the build go?

jjonescz · 2026-01-28T10:12:30Z

There is one regression in Speedometer

but it looks like noise

CyrusNajmabadi · 2026-02-14T10:29:48Z

@dotnet/roslyn-compiler ptal.

333fred

Overall LGTM, but I do have a question on hash distribution. I think the existing behavior is the same, but I'm curious nonetheless.

333fred · 2026-02-26T00:52:33Z

src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCache.cs

-
-        private static readonly Entry[] s_cache = new Entry[CacheSize];
+        /// <summary>
+        /// Simply array indexed by the hash of the cached node.  Note that unlike a typical dictionary/hashtable, this


Suggested change

/// Simply array indexed by the hash of the cached node. Note that unlike a typical dictionary/hashtable, this

/// Simple array indexed by the hash of the cached node. Note that unlike a typical dictionary/hashtable, this

333fred · 2026-02-26T00:56:27Z

src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCache.cs


-            int hash = child.GetCacheHash();
+            int hash = GetCacheHash(child);
            int idx = hash & CacheMask;


It feels like nothing actually ensures there's a uniform distribution here. Is that a correct intuition? Is there anything we could do to ensure that?

Removing seemingly unnecessary data from cache

efce2b2

github-actions bot added the Area-Compilers label Oct 20, 2025

CyrusNajmabadi added 2 commits October 21, 2025 11:17

Add docs

f95e863

Simplify logic

4d5d748

CyrusNajmabadi changed the title ~~Removing seemingly unnecessary data from cache~~ Removing unnecessary checks from SyntaxNodeCache Oct 21, 2025

CyrusNajmabadi commented Oct 21, 2025

View reviewed changes

src/Compilers/Core/Portable/Syntax/GreenNode.cs Show resolved Hide resolved

Cleanup

8cd1520

CyrusNajmabadi commented Oct 21, 2025

View reviewed changes

src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCache.cs Outdated Show resolved Hide resolved

Update src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCa…

77ebcde

…che.cs

CyrusNajmabadi commented Oct 21, 2025

View reviewed changes

CyrusNajmabadi added 2 commits October 21, 2025 11:47

Update tests

5d12590

Merge branch 'removeHash' of https://github.com/CyrusNajmabadi/roslyn …

5f04d74

…into removeHash

CyrusNajmabadi force-pushed the removeHash branch from 5365f57 to 5f04d74 Compare October 21, 2025 10:18

CyrusNajmabadi changed the title ~~Removing unnecessary checks from SyntaxNodeCache~~ Simplifying internal structure and logic for the SyntaxNodeCache Oct 21, 2025

CyrusNajmabadi marked this pull request as ready for review October 21, 2025 10:24

CyrusNajmabadi requested a review from a team as a code owner October 21, 2025 10:24

CyrusNajmabadi requested a review from ToddGrun October 21, 2025 10:24

CyrusNajmabadi force-pushed the removeHash branch from c39989c to 5f04d74 Compare October 21, 2025 10:29

CyrusNajmabadi added 2 commits November 10, 2025 22:05

Merge remote-tracking branch 'upstream/main' into removeHash

d4989bb

Merge remote-tracking branch 'upstream/main' into removeHash

411b229

CyrusNajmabadi commented Nov 11, 2025

View reviewed changes

CyrusNajmabadi marked this pull request as draft November 11, 2025 20:22

CyrusNajmabadi added 2 commits November 11, 2025 21:22

Merge remote-tracking branch 'upstream/main' into removeHash

6bc244b

Restore check

891a8cb

CyrusNajmabadi marked this pull request as ready for review November 12, 2025 11:37

jjonescz approved these changes Jan 23, 2026

View reviewed changes

CyrusNajmabadi and others added 5 commits January 23, 2026 14:42

Merge remote-tracking branch 'upstream/main' into removeHash

f4e8121

Update src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCa…

40c5796

…che.cs Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

Update src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCa…

c14525a

…che.cs Co-authored-by: Jan Jones <jan.jones.cz@gmail.com>

remove unnecessary import

ac3fa11

Merge branch 'removeHash' of https://github.com/CyrusNajmabadi/roslyn …

30a6a9f

…into removeHash

	/// Simply array indexed by the hash of the cached node. Note that unlike a typical dictionary/hashtable, this
	/// Simple array indexed by the hash of the cached node. Note that unlike a typical dictionary/hashtable, this

Conversation

CyrusNajmabadi commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Oct 21, 2025

Uh oh!

CyrusNajmabadi Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Nov 30, 2025

Uh oh!

CyrusNajmabadi commented Nov 30, 2025

Uh oh!

ToddGrun commented Nov 30, 2025

Uh oh!

CyrusNajmabadi commented Jan 15, 2026

Uh oh!

CyrusNajmabadi commented Jan 20, 2026

Uh oh!

CyrusNajmabadi commented Jan 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Jan 23, 2026

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as resolved.

github-actions bot commented Jan 23, 2026 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Jan 27, 2026

Uh oh!

jjonescz commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi commented Feb 14, 2026 • edited by jjonescz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

333fred Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

333fred Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CyrusNajmabadi commented Oct 20, 2025 •

edited

Loading

CyrusNajmabadi Oct 21, 2025 •

edited

Loading

CyrusNajmabadi Oct 21, 2025 •

edited

Loading

github-actions bot commented Jan 23, 2026 •

edited by jjonescz

Loading

jjonescz commented Jan 28, 2026 •

edited

Loading

CyrusNajmabadi commented Feb 14, 2026 •

edited by jjonescz

Loading