Skip to content
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

Reduce allocations for SyntaxListPool and SyntaxListBuilder #76685

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Reduce allocations for SyntaxListPool and SyntaxListBuilder #76685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c6fc533
WIP: Add a pool around SyntaxListPool
ToddGrun Jan 8, 2025
9739e80
Protect SyntaxListBuilder.Count call against the underlying builder b…
ToddGrun Jan 8, 2025
d79d3d9
Merge branch 'dotnet:main' into dev/toddgrun/PoolSyntaxListPool
ToddGrun Jan 16, 2025
7fd6dac
add sealed
ToddGrun Jan 16, 2025
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
9 changes: 8 additions & 1 deletion src/Compilers/CSharp/Portable/Parser/DocumentationCommentParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,21 @@ namespace Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax

internal sealed class DocumentationCommentParser : SyntaxParser
{
private readonly SyntaxListPool _pool = new SyntaxListPool();
private readonly SyntaxListPool _pool = SyntaxListPool.GetInstance();
private bool _isDelimited;

internal DocumentationCommentParser(Lexer lexer)
: base(lexer, LexerMode.None, oldTree: null, changes: null, allowModeReset: true)
{
}

public override void Dispose()
{
base.Dispose();

_pool.Free();
}

internal void ReInitialize(LexerMode modeflags)
{
base.ReInitialize();
Expand Down
9 changes: 8 additions & 1 deletion src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ internal sealed partial class LanguageParser : SyntaxParser
// can be reused (hence pooled) since the syntax factory methods don't keep references to
// them

private readonly SyntaxListPool _pool = new SyntaxListPool(); // Don't need to reset this.
private readonly SyntaxListPool _pool = SyntaxListPool.GetInstance(); // Don't need to reset this.
Copy link
Member

Choose a reason for hiding this comment

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

Trying to understand if the comment here is still valid. There is no resetting in the pool and it's a ctor call all the time. But the comment had me digging into the pool to see if we now needed to reset state since there was a pool and not always a new.

@cston, @CyrusNajmabadi


private readonly SyntaxFactoryContext _syntaxFactoryContext; // Fields are resettable.
private readonly ContextAwareSyntax _syntaxFactory; // Has context, the fields of which are resettable.
Expand All @@ -46,6 +46,13 @@ internal LanguageParser(
_syntaxFactory = new ContextAwareSyntax(_syntaxFactoryContext);
}

public override void Dispose()
{
base.Dispose();

_pool.Free();
}

private static bool IsSomeWord(SyntaxKind kind)
{
return kind == SyntaxKind.IdentifierToken || SyntaxFacts.IsKeywordKind(kind);
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Parser/SyntaxParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ protected SyntaxParser(
}
}

public void Dispose()
public virtual void Dispose()
{
var blendedTokens = _blendedTokens;
if (blendedTokens != null)
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxListBuilder`1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public int Count
{
get
{
return _builder.Count;
return _builder?.Count ?? 0;
}
}

Expand Down
53 changes: 41 additions & 12 deletions src/Compilers/Core/Portable/Syntax/InternalSyntax/SyntaxListPool.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,43 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

// #define LOG

using System;
#if LOG
using System.Collections.Generic;
using System.Diagnostics;
using Roslyn.Utilities;
#endif
using Microsoft.CodeAnalysis.PooledObjects;

namespace Microsoft.CodeAnalysis.Syntax.InternalSyntax
{
internal class SyntaxListPool
internal sealed class SyntaxListPool
{
private ArrayElement<SyntaxListBuilder?>[] _freeList = new ArrayElement<SyntaxListBuilder?>[10];
private static readonly ObjectPool<SyntaxListPool> s_listPool = new ObjectPool<SyntaxListPool>(() => new SyntaxListPool());

private const int InitialFreeListSize = 16;
private const int InitialBuilderCapacity = 32;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I played around with these numbers a bit and these seemed to work well for the Roslyn sln


private ArrayElement<SyntaxListBuilder?>[] _freeList = new ArrayElement<SyntaxListBuilder?>[InitialFreeListSize];
private int _freeIndex;

#if DEBUG
#if LOG
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is useful, but only when you are really looking into this code. The overhead seems high enough that it's not worth doing unless it's being investigated, thus the removal from vanilla debug builds.

private readonly List<SyntaxListBuilder> _allocated = new List<SyntaxListBuilder>();
#endif

internal SyntaxListPool()
private SyntaxListPool()
{
}

public static SyntaxListPool GetInstance()
{
return s_listPool.Allocate();
}

public void Free()
{
s_listPool.Free(this);
Copy link
Member

Choose a reason for hiding this comment

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

One part of the change I'm having trouble understanding is that before it was always new SyntaxListPool. That meant every time it started with _freeIndex at 0. There is no resetting of that in GetInstance or Free here, nor is there an assertion that we are at _freeIndex == 0 in the Free method. Hence it seems to me that we could get in a case where an instance is taken out of the pool where _freeIndex != 0 ...

Am I missing something about the change here?

Copy link
Contributor Author

@ToddGrun ToddGrun Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think what might be confusing is there is a pool of SyntaxListPools each of which is a pool of SyntaxListBuilders. The GetInstance()/Free() methods operate on the pool of SyntaxListPools. The Allocate*/Free(SyntaxListBuilder) methods operate on the pool of SyntaxListBuilders.

_freeIndex represents an index into a SyntaxListPool's pool of SyntaxListBuilder objects. When this value is zero, it indicates there isn't a SyntaxListBuilder in the SyntaxListPool, and a new one needs to be allocated. Because we are pooling these SyntaxListBuilders inside our SyntaxListPool, we don't want to reset _freeIndex when adding this SyntaxListPool back to the SyntaxListPool pool.

Maybe having both the pool of SyntaxListPools and pool of SyntaxListBuilders in the same class is leading to confusion?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe having both the pool of SyntaxListPools and pool of SyntaxListBuilders in the same class is leading to confusion?

For me at least it is. I'm still stuck on the idea that we could free a SyntaxListPool but the underlying pool of SyntaxListBuilder is not guaranteed to be free.

Copy link
Contributor Author

@ToddGrun ToddGrun Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The very first thing I tried when I noticed these allocations was to simplify the code by using a more standard usage of ObjectPool. However, there was just too much contention across simultaneous parses to use a single ObjectPool.

That convinced me that having multiple SyntaxListPool objects provides value. So, I'm fairly comfortable with pooling SyntaxListPool objects.

The reason I added the ObjectPool in SyntaxListPool itself was because we used this pattern where a class encapsulates the pooling of itself in a recent compiler PR that I made (also, I wasn't sure where else would be a good place for this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jaredpar -- Does the latest explanation clear it up a bit? If not, what change would make it more clear?

}

internal SyntaxListBuilder Allocate()
Expand All @@ -33,10 +52,10 @@ internal SyntaxListBuilder Allocate()
}
else
{
item = new SyntaxListBuilder(10);
item = new SyntaxListBuilder(InitialBuilderCapacity);
}

#if DEBUG
#if LOG
Debug.Assert(!_allocated.Contains(item));
_allocated.Add(item);
#endif
Expand All @@ -63,16 +82,26 @@ internal void Free(SyntaxListBuilder? item)
if (item is null)
return;

item.Clear();
#if LOG
Debug.Assert(_allocated.Contains(item));

_allocated.Remove(item);
#endif

// Don't add the builder back to _freelist if the builder has grown too large.
if (item.Capacity > InitialBuilderCapacity * 2)
return;

if (_freeIndex >= _freeList.Length)
{
// Don't add the builder back to _freelist if the cache has grown too large.
if (_freeIndex >= InitialFreeListSize * 2)
return;

this.Grow();
}
#if DEBUG
Debug.Assert(_allocated.Contains(item));

_allocated.Remove(item);
#endif
item.Clear();
_freeList[_freeIndex].Value = item;
_freeIndex++;
}
Expand Down
5 changes: 4 additions & 1 deletion src/Compilers/VisualBasic/Portable/Parser/BlockContexts/BlockContext.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,10 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
End Property

Friend Sub FreeStatements()
_parser._pool.Free(_statements)
If Not _statements.IsNull Then
_parser._pool.Free(_statements)
_statements = Nothing
Copy link
Contributor Author

Choose a reason for hiding this comment

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

_statements = Nothing

Added this so the code doesn't fall apart if FreeStatements is called multiple times

End If
End Sub

Friend Function Body() As CodeAnalysis.Syntax.InternalSyntax.SyntaxList(Of StatementSyntax)
Expand Down
7 changes: 6 additions & 1 deletion src/Compilers/VisualBasic/Portable/Parser/Parser.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
Private _evaluatingConditionCompilationExpression As Boolean
Private ReadOnly _scanner As Scanner
Private ReadOnly _cancellationToken As CancellationToken
Friend ReadOnly _pool As New SyntaxListPool
Friend ReadOnly _pool As SyntaxListPool = SyntaxListPool.GetInstance()
Private ReadOnly _syntaxFactory As ContextAwareSyntaxFactory

' When parser owns the scanner, it is responsible for disposing it
Expand Down Expand Up @@ -68,6 +68,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
If _disposeScanner Then
Me._scanner.Dispose()
End If

' Ensure the context's statements have been freed
Context?.FreeStatements()

_pool.Free()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why I had the earlier PR to ensure that the VB parser's dispose is always created

End Sub

Friend ReadOnly Property IsScript As Boolean
Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/VisualBasic/Portable/Scanner/Scanner.vb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@ Imports System.Globalization
Imports System.Runtime.InteropServices
Imports System.Text
Imports Microsoft.CodeAnalysis.PooledObjects
Imports Microsoft.CodeAnalysis.Syntax.InternalSyntax
Imports Microsoft.CodeAnalysis.Text
Imports Microsoft.CodeAnalysis.VisualBasic
Imports Microsoft.CodeAnalysis.VisualBasic.SyntaxFacts
Imports Microsoft.CodeAnalysis.Collections
Imports Microsoft.CodeAnalysis.Syntax.InternalSyntax
Imports CoreInternalSyntax = Microsoft.CodeAnalysis.Syntax.InternalSyntax

Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
Expand Down Expand Up @@ -46,7 +45,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
''' USE GetScratch()
''' </summary>
Private ReadOnly _sb As StringBuilder = _sbPooled.Builder
Private ReadOnly _triviaListPool As New SyntaxListPool
Private ReadOnly _triviaListPool As SyntaxListPool = SyntaxListPool.GetInstance()
Private ReadOnly _options As VisualBasicParseOptions

Private ReadOnly _stringTable As StringTable = StringTable.GetInstance()
Expand Down Expand Up @@ -124,6 +123,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax
_quickTokenTable.Free()
_stringTable.Free()
_sbPooled.Free()
_triviaListPool.Free()

s_idTablePool.Free(_idTable)
s_kwTablePool.Free(_kwTable)
Expand Down