Skip to content

Simplifying internal structure and logic for the SyntaxNodeCache #80825

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.

Sign up for GitHub

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

Merged
jjonescz merged 17 commits into from
Mar 18, 2026
Merged

Simplifying internal structure and logic for the SyntaxNodeCache #80825

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
efce2b2
Removing seemingly unnecessary data from cache
CyrusNajmabadi Oct 20, 2025
f95e863
Add docs
CyrusNajmabadi Oct 21, 2025
4d5d748
Simplify logic
CyrusNajmabadi Oct 21, 2025
8cd1520
Cleanup
CyrusNajmabadi Oct 21, 2025
77ebcde
Update src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCa…
CyrusNajmabadi Oct 21, 2025
5d12590
Update tests
CyrusNajmabadi Oct 21, 2025
5f04d74
Merge branch 'removeHash' of https://github.com/CyrusNajmabadi/roslyn…
CyrusNajmabadi Oct 21, 2025
d4989bb
Merge remote-tracking branch 'upstream/main' into removeHash
CyrusNajmabadi Nov 10, 2025
411b229
Merge remote-tracking branch 'upstream/main' into removeHash
CyrusNajmabadi Nov 11, 2025
6bc244b
Merge remote-tracking branch 'upstream/main' into removeHash
CyrusNajmabadi Nov 11, 2025
891a8cb
Restore check
CyrusNajmabadi Nov 11, 2025
f4e8121
Merge remote-tracking branch 'upstream/main' into removeHash
CyrusNajmabadi Jan 23, 2026
40c5796
Update src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCa…
CyrusNajmabadi Jan 23, 2026
c14525a
Update src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCa…
CyrusNajmabadi Jan 23, 2026
ac3fa11
remove unnecessary import
CyrusNajmabadi Jan 23, 2026
30a6a9f
Merge branch 'removeHash' of https://github.com/CyrusNajmabadi/roslyn…
CyrusNajmabadi Jan 23, 2026
8ffd9dd
Update src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCa…
CyrusNajmabadi Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 0 additions & 65 deletions src/Compilers/Core/Portable/Syntax/GreenNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -962,71 +962,6 @@ public SyntaxNode CreateRed()

#endregion

#region Caching

@CyrusNajmabadi CyrusNajmabadi Oct 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this code was moved. with no changes.


internal const int MaxCachedChildNum = 3;

internal bool IsCacheable
{
get
{
return ((this.Flags & NodeFlags.InheritMask) == NodeFlags.IsNotMissing) &&
this.SlotCount <= GreenNode.MaxCachedChildNum;
}
}

internal int GetCacheHash()
{
Debug.Assert(this.IsCacheable);

int code = (int)(this.Flags) ^ this.RawKind;
int cnt = this.SlotCount;
for (int i = 0; i < cnt; i++)
{
var child = GetSlot(i);
if (child != null)
{
code = Hash.Combine(RuntimeHelpers.GetHashCode(child), code);
}
}

return code & Int32.MaxValue;
}

internal bool IsCacheEquivalent(int kind, NodeFlags flags, GreenNode? child1)
{
Debug.Assert(this.IsCacheable);

return this.RawKind == kind &&
this.Flags == flags &&
this.SlotCount == 1 &&
Comment thread
CyrusNajmabadi marked this conversation as resolved.
this.GetSlot(0) == child1;
}

internal bool IsCacheEquivalent(int kind, NodeFlags flags, GreenNode? child1, GreenNode? child2)
{
Debug.Assert(this.IsCacheable);

return this.RawKind == kind &&
this.Flags == flags &&
this.SlotCount == 2 &&
this.GetSlot(0) == child1 &&
this.GetSlot(1) == child2;
}

internal bool IsCacheEquivalent(int kind, NodeFlags flags, GreenNode? child1, GreenNode? child2, GreenNode? child3)
{
Debug.Assert(this.IsCacheable);

return this.RawKind == kind &&
this.Flags == flags &&
this.SlotCount == 3 &&
this.GetSlot(0) == child1 &&
this.GetSlot(1) == child2 &&
this.GetSlot(2) == child3;
}
#endregion //Caching

/// <summary>
/// Add an error to the given node, creating a new node that is the same except it has no parent,
/// and has the given error attached to it. The error span is the entire span of this node.
Expand Down
154 changes: 121 additions & 33 deletions src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxNodeCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.Syntax.InternalSyntax;

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using Roslyn.Utilities;

#if STATS
Expand All @@ -15,6 +14,7 @@ namespace Microsoft.CodeAnalysis.Syntax.InternalSyntax
{
/// <summary>
/// Provides caching functionality for green nonterminals with up to 3 children.
///
Comment thread
CyrusNajmabadi marked this conversation as resolved.
Outdated
/// Example:
/// When constructing a node with given kind, flags, child1 and child2, we can look up
/// in the cache whether we already have a node that contains same kind, flags,
Expand All @@ -38,8 +38,21 @@ namespace Microsoft.CodeAnalysis.Syntax.InternalSyntax
/// We only consider "normal" nodes to be cacheable.
/// Nodes with diagnostics/annotations/directives/skipped, etc... have more complicated identity
/// and are not likely to be repetitive.
///
/// </summary>
/// <remarks>
/// The use of <see cref="GreenNode.RawKind"/>, <see cref="GreenNode.NodeFlags"/> (and any provided child nodes)
/// ensures during lookup that we only return a node that is identical in all relevant aspects to the node that
/// we're about to create otherwise. This is a brittle guarantee. However, given how locked down green nodes are,
/// this seems acceptable for now. Great care needs to be taken if new properties are added to green nodes that
/// would affect their identity.
/// <para/>
/// Only nodes created through SyntaxFactory methods are cached. Nodes created directly through their constructors
/// are not cached. This is fairly intuitive as a constructor would not be able to somehow return some other
/// instance different than the one being constructed. A subtle aspect of this though is that this is what ensures
/// that nodes with diagnostics or annotations on them are not cached. These nodes are created starting with
/// another node and forking it to add diagnostics/annotations. This forking always calls through a constructor and
/// not a factory method. And as such, never comes through here.
/// </remarks>
internal class GreenStats
{
// TODO: remove when done tweaking this cache.
Expand Down Expand Up @@ -114,36 +127,31 @@ internal static class SyntaxNodeCache
private const int CacheSize = 1 << CacheSizeBits;
private const int CacheMask = CacheSize - 1;

private readonly struct Entry
{
public readonly int hash;
public readonly GreenNode? node;

internal Entry(int hash, GreenNode node)
{
this.hash = hash;
this.node = node;
}
}

@CyrusNajmabadi CyrusNajmabadi Oct 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


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
Comment thread
CyrusNajmabadi marked this conversation as resolved.
Outdated
/// does not exercise any form of collision resolution. If two different nodes hash to the same index, the
/// latter will overwrite the former. This is acceptable since this is just an opportunistic cache. Reads from
/// the cache validate that the node they get back is actually the one they were looking for. See the comments
/// in <see cref="TryGetNode(int, GreenNode?, out int)"/> for more details.
/// </summary>
private static readonly GreenNode[] s_cache = new GreenNode[CacheSize];

internal static void AddNode(GreenNode node, int hash)
{
if (AllChildrenInCache(node) && !node.IsMissing)
{
GreenStats.ItemAdded();

Debug.Assert(node.GetCacheHash() == hash);
Debug.Assert(GetCacheHash(node) == hash);

var idx = hash & CacheMask;
s_cache[idx] = new Entry(hash, node);
s_cache[idx] = node;
}
}

private static bool CanBeCached(GreenNode? child1)
{
return child1 == null || child1.IsCacheable;
return child1 == null || IsCacheable(child1);
}

private static bool CanBeCached(GreenNode? child1, GreenNode? child2)
Expand All @@ -163,9 +171,9 @@ private static bool ChildInCache(GreenNode? child)
// TODO: should use slotCount
if (child == null || child.SlotCount == 0) return true;

int hash = child.GetCacheHash();
int hash = GetCacheHash(child);
int idx = hash & CacheMask;
Comment thread
333fred marked this conversation as resolved.
return s_cache[idx].node == child;
return s_cache[idx] == child;
}

private static bool AllChildrenInCache(GreenNode node)
Expand Down Expand Up @@ -194,13 +202,18 @@ private static bool AllChildrenInCache(GreenNode node)
{
GreenStats.ItemCacheable();

// Determine the hash for the node being created, given its kind, flags, and optional single child. Then
// grab out a potential cached node from the cache based on that hash. Note that this may not actually
// be a viable match due to potential hash collisions, where we have 'last one wins' semantics. So if
// we do see a node in the cache, we have to validate that it is actually equivalent to the the data
Comment thread
CyrusNajmabadi marked this conversation as resolved.
Outdated
// being used populate the cache entry. This is what IsCacheEquivalent is for. It allows us to check
Comment thread
CyrusNajmabadi marked this conversation as resolved.
Outdated
// that the node already there has that same kind, flags, and the same child (by reference).
int h = hash = GetCacheHash(kind, flags, child1);
int idx = h & CacheMask;
var e = s_cache[idx];
if (e.hash == h && e.node != null && e.node.IsCacheEquivalent(kind, flags, child1))
var e = s_cache[h & CacheMask];
if (IsCacheEquivalent(e, kind, flags, child1))
{
GreenStats.CacheHit();
return e.node;
return e;
}
}
else
Expand All @@ -223,12 +236,11 @@ private static bool AllChildrenInCache(GreenNode node)
GreenStats.ItemCacheable();

int h = hash = GetCacheHash(kind, flags, child1, child2);
int idx = h & CacheMask;
var e = s_cache[idx];
if (e.hash == h && e.node != null && e.node.IsCacheEquivalent(kind, flags, child1, child2))
var e = s_cache[h & CacheMask];
if (IsCacheEquivalent(e, kind, flags, child1, child2))
{
GreenStats.CacheHit();
return e.node;
return e;
}
}
else
Expand All @@ -251,12 +263,11 @@ private static bool AllChildrenInCache(GreenNode node)
GreenStats.ItemCacheable();

int h = hash = GetCacheHash(kind, flags, child1, child2, child3);
int idx = h & CacheMask;
var e = s_cache[idx];
if (e.hash == h && e.node != null && e.node.IsCacheEquivalent(kind, flags, child1, child2, child3))
var e = s_cache[h & CacheMask];
if (IsCacheEquivalent(e, kind, flags, child1, child2, child3))
{
GreenStats.CacheHit();
return e.node;
return e;
}
}
else
Expand Down Expand Up @@ -318,5 +329,82 @@ private static int GetCacheHash(int kind, GreenNode.NodeFlags flags, GreenNode?
// ensure nonnegative hash
return code & Int32.MaxValue;
}

private const int MaxCachedChildNum = 3;

private static bool IsCacheable(GreenNode node)
{
return ((node.Flags & GreenNode.NodeFlags.InheritMask) == GreenNode.NodeFlags.IsNotMissing) &&
node.SlotCount <= MaxCachedChildNum;
}

private static int GetCacheHash(GreenNode node)
{
Debug.Assert(IsCacheable(node));

int code = (int)(node.Flags) ^ node.RawKind;
int cnt = node.SlotCount;
for (int i = 0; i < cnt; i++)
{
var child = node.GetSlot(i);
if (child != null)
{
code = Hash.Combine(RuntimeHelpers.GetHashCode(child), code);
}
}

return code & Int32.MaxValue;
}

private static bool IsCacheEquivalent(GreenNode? parent, int kind, GreenNode.NodeFlags flags, GreenNode? child1)
{
if (parent is null)
return false;

Debug.Assert(IsCacheable(parent));

// If the parent kind matches, the slot count must match as well. Only if the parent kind does not match
// (so two different node kinds hashed to the same array location) could the slot counts differ.
Debug.Assert(parent.RawKind != kind || parent.SlotCount == 1);

return parent.RawKind == kind &&
parent.Flags == flags &&
parent.GetSlot(0) == child1;
}

private static bool IsCacheEquivalent(GreenNode? parent, int kind, GreenNode.NodeFlags flags, GreenNode? child1, GreenNode? child2)
{
if (parent is null)
return false;

Debug.Assert(IsCacheable(parent));

// If the parent kind matches, the slot count must match as well. Only if the parent kind does not match
// (so two different node kinds hashed to the same array location) could the slot counts differ.
Debug.Assert(parent.RawKind != kind || parent.SlotCount == 2);

return parent.RawKind == kind &&
parent.Flags == flags &&
parent.GetSlot(0) == child1 &&
parent.GetSlot(1) == child2;
}

private static bool IsCacheEquivalent(GreenNode? parent, int kind, GreenNode.NodeFlags flags, GreenNode? child1, GreenNode? child2, GreenNode? child3)
{
if (parent is null)
return false;

Debug.Assert(IsCacheable(parent));

// If the parent kind matches, the slot count must match as well. Only if the parent kind does not match
// (so two different node kinds hashed to the same array location) could the slot counts differ.
Debug.Assert(parent.RawKind != kind || parent.SlotCount == 2);

return parent.RawKind == kind &&
parent.Flags == flags &&
parent.GetSlot(0) == child1 &&
parent.GetSlot(1) == child2 &&
parent.GetSlot(2) == child3;
}
}
}
Loading