Skip to content

Reduce allocation from data taken in local performance trace#48896

Merged
sharwell merged 6 commits into
dotnet:masterfrom
sharwell:reduce-allocs
Oct 28, 2020
Merged

Reduce allocation from data taken in local performance trace#48896
sharwell merged 6 commits into
dotnet:masterfrom
sharwell:reduce-allocs

Conversation

@sharwell
Copy link
Copy Markdown
Contributor

@sharwell sharwell commented Oct 24, 2020

I can separate refactoring 7e6b174 if desired.

Comment thread src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs Outdated
Comment thread src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs Outdated
@CyrusNajmabadi

This comment has been minimized.

@sharwell sharwell marked this pull request as draft October 24, 2020 07:46
@sharwell

This comment has been minimized.

@sharwell

This comment has been minimized.

Comment thread src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.cs Outdated
Comment thread src/Compilers/Core/Portable/CaseInsensitiveComparison.cs Outdated
Comment thread src/Workspaces/Core/Portable/FindSymbols/SymbolTree/SymbolTreeInfo.Node.cs Outdated
@sharwell sharwell marked this pull request as ready for review October 27, 2020 06:50
@sharwell sharwell requested a review from a team as a code owner October 27, 2020 06:50
@sharwell
Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi @jaredpar this is now cleaned up and rebased. The only content changes in the rebase were the final two commits, which can be reviewed per-commit.

@dotnet/roslyn-compiler I will need a second review here in addition to the one from Jared.

Comment thread src/Compilers/Core/Portable/PublicAPI.Shipped.txt Outdated
@AlekseyTs

This comment has been minimized.

@AlekseyTs

This comment has been minimized.

Comment thread src/Dependencies/PooledObjects/ArrayBuilder.cs Outdated
@AlekseyTs

This comment has been minimized.

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.

Changes under Compilers LGTM (iteration 6)

Comment thread src/Workspaces/CoreTest/Collections/TemporaryArrayTests.cs Outdated
Comment thread src/Workspaces/CoreTest/Collections/TemporaryArrayTests.cs Outdated
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

very nice!

@sharwell
Copy link
Copy Markdown
Contributor Author

@jaredpar This just needs a final sign-off from you now

/// <typeparam name="T">The type of elements stored in the collection.</typeparam>
[NonCopyable]
[StructLayout(LayoutKind.Sequential)]
internal struct TemporaryArray<T> : IDisposable
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.

@sharwell I'm wondering if it is at all possible / preferable to use InlineArray for this?

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.

@alrz InlineArray wouldn't be a complete replacement for this, but it would certainly be a viable replacement for the _item0..._item3 fields.

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.

10 participants