Skip to content

Prefer stack storage for highly-contended temporary array#61670

Merged
sharwell merged 1 commit intodotnet:mainfrom
sharwell:thread-local
Jun 7, 2022
Merged

Prefer stack storage for highly-contended temporary array#61670
sharwell merged 1 commit intodotnet:mainfrom
sharwell:thread-local

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Jun 2, 2022

Fixes AB#1449857

@sharwell sharwell requested review from a team as code owners June 2, 2022 21:35
@ghost ghost added the Area-Compilers label Jun 2, 2022
var uninst1 = ArrayBuilder<TypeSymbol>.GetInstance();
var uninst2 = ArrayBuilder<TypeSymbol>.GetInstance();
using var uninst1 = TemporaryArray<TypeSymbol>.Empty;
using var uninst2 = TemporaryArray<TypeSymbol>.Empty;
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.

📝 TemporaryArray<T> stores up to 4 elements on the stack, and only uses ArrayBuilder<T>.GetInstance() for 5 or more elements.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

unrelated, have we looked at all at making these pools thread-local?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Being pinned to a core might make sense as well.

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.

There are options there, but it's difficult to implement them without penalizing some other scenario. Stack-based allocations have done a good job of addressing this situation in IDE code, so I would expect them to do the same here.

@genlu
Copy link
Copy Markdown
Member

genlu commented Jun 2, 2022

Hey @sharwell, do you mind explaining what did you see in the trace prompted this change? Thanks!

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jun 2, 2022

@genlu The trace shows high CPU usage in ObjectPool`1[System.__Canon].Allocate(). I've seen this in the past on my 2990WX, and has to do with Interlocked.CompareExchange needing to synchronize the caches of all 64 processors. The fix reduces contention on the shared pool by reducing the number of calls that need to use the pool (most will now use the current stack instead).

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 1)

@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jun 6, 2022

@dotnet/roslyn-compiler for second review

1 similar comment
@sharwell
Copy link
Copy Markdown
Contributor Author

sharwell commented Jun 7, 2022

@dotnet/roslyn-compiler for second review

@sharwell sharwell merged commit 0d350fb into dotnet:main Jun 7, 2022
@sharwell sharwell deleted the thread-local branch June 7, 2022 15:05
@ghost ghost added this to the Next milestone Jun 7, 2022
@RikkiGibson RikkiGibson modified the milestones: Next, 17.3 P3 Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants