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.

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 in the C# lexer ctor #76544

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ToddGrun
Copy link
Contributor

@ToddGrun ToddGrun commented Dec 20, 2024

The lexer ctor shows as about 0.6% of all allocations over the lifetime of the RoslynCodeAnalysisProcess in the csharp editing speedometer tests.

There was already the concept of the LexerCache which had some pooling usage in it. This PR moves a couple other members to it to allow for their pooling.

Speedometer results look quite promising:

Allocations under Lexer.ctor in IncPaths reduced from 60 MB to 1.5 MB
Allocations under Lexer in IncPaths reduced from 1.57 GB to 1.36 GB

The lexer ctor shows as about 0.5% of all allocations over the lifetime of the RoslynCodeAnalysisProcess in the csharp editing speedometer tests.

There was already the concept of the LexerCache which had some pooling usage in it. This PR moves a couple other members to it to allow for their pooling.

This PR is going to be marked as a draft PR until I can get some data from a test insertion indicating whether these changes improve allocations in the speedometer tests.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Dec 20, 2024
@ToddGrun
Copy link
Contributor Author

ToddGrun commented Dec 20, 2024

@ToddGrun
Copy link
Contributor Author

Speedometer results look promising, going to promote out of draft mode.

Allocations under Lexer.*ctor in IncPaths:
60 MB -> 1.5 MB
Allocations under Lexer in IncPaths:
1,57 GB -> 1.36 GB

@ToddGrun ToddGrun changed the title WIP: Attempt to reduce allocations in the C# lexer ctor Reduce allocations in the C# lexer ctor Dec 21, 2024
@ToddGrun ToddGrun marked this pull request as ready for review December 21, 2024 13:59
@ToddGrun ToddGrun requested a review from a team as a code owner December 21, 2024 13:59
@ToddGrun
Copy link
Contributor Author

@dotnet/roslyn-compiler -- ptal

@@ -24,6 +25,7 @@ public static SyntaxListBuilder Create()

public void Clear()
{
Array.Clear(_nodes, 0, Count);
Copy link
Contributor

Choose a reason for hiding this comment

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

Array.Clear(_nodes, 0, Count);

Why not clear the entire array? It looks like the indexer below doesn't check/adjust the Count

}

internal void Free()
{
_keywordKindMap.Free();
_keywordKindMap = s_keywordKindPool.Allocate();
Copy link
Contributor

Choose a reason for hiding this comment

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

_keywordKindMap = s_keywordKindPool.Allocate();

It feels counterintuitive that a Free method doesn't actually free, but allocates. If we need a clear method, let's add it or rename this one if we would like to change its purpose.

@@ -70,6 +70,8 @@ internal enum XmlDocCommentStyle

internal sealed partial class Lexer : AbstractLexer
{
private static readonly ObjectPool<LexerCache> s_lexerCachePool = new ObjectPool<LexerCache>(() => new LexerCache());
Copy link
Contributor

Choose a reason for hiding this comment

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

s_lexerCachePool

Since we are making LexerCache pollable, would it make sense to encapsulate its pool in LexerCache?


private const int LeadingTriviaCacheInitialCapacity = 128;
private const int TrailingTriviaCacheInitialCapacity = 16;

internal LexerCache()
Copy link
Contributor

Choose a reason for hiding this comment

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

internal

private?

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants