Reuse prior TableEntry values in the increment NodeTable builder if possible. #62320
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CyrusNajmabadi
commented
Jul 1, 2022
| ? (previous, EntryState.Cached) | ||
| : (replacement, EntryState.Modified); | ||
| ? (previous, EntryState.Cached, chosePrevious: true) | ||
| : (replacement, EntryState.Modified, chosePrevious: false); |
There was a problem hiding this comment.
note: chosePrevious is technically redundant. we can tell waht happened by seeing if we got EntryState.Cached or EntryState.Modified back. But i was worreid that was too confusing/brittle, so i went this path.
CyrusNajmabadi
commented
Jul 1, 2022
| @@ -578,15 +620,24 @@ public void Add(T item, EntryState state) | |||
| } | |||
| else if (_currentState != state) | |||
| { | |||
| _states = ArrayBuilder<EntryState>.GetInstance(_items.Count - 1, _currentState.Value); | |||
There was a problem hiding this comment.
this approach was an anti pattern. by doing GetInstance(_items.Count - 1) we were always allocating an array one too small for what we needed. The adding a value would force us to double and then copy and then throw that initial array away.
The new form both avoids that, and avoids any wastage entirely by ensuring we use _requestedCapacity to get exactly the size we need.
333fred
reviewed
Jul 1, 2022
|
@dotnet/roslyn-compiler for reviews please :) |
333fred
approved these changes
Jul 5, 2022
|
@jcouv can you ptal? thanks! |
jcouv
reviewed
Jul 6, 2022
| private EntryState? _currentState; | ||
|
|
||
| private readonly int _requestedCapacity; |
There was a problem hiding this comment.
This is only used in DEBUG mode for assertions. Can we if/def?
Ignore that. I'd missed a usage. #Closed
CyrusNajmabadi
commented
Jul 6, 2022
src/Compilers/Core/Portable/SourceGeneration/Nodes/NodeStateTable.cs
Outdated
Show resolved
Hide resolved
jcouv
reviewed
Jul 6, 2022
src/Compilers/Core/Portable/SourceGeneration/Nodes/NodeStateTable.cs
Outdated
Show resolved
Hide resolved
ghost
added this to the
333fred
added a commit
to 333fred/roslyn
that referenced
this pull request
Jul 7, 2022
* upstream/main: (62 commits) Prevent assert from being hit (dotnet#62366) Don't offer '??=' for pointer types (dotnet#62476) Integrate generator times into /reportAnalyzer (dotnet#61661) Switch to a callback for cleaning up resources instead of passing in an explicit IDisposable. (dotnet#62373) Filter trees to only those containing global-usings or attributes prior to analyzing them. (dotnet#62444) Update PublishData.json Complete 'file' support for `SyntaxGenerator` (dotnet#62413) Apply changes directly to text buffer (dotnet#62337) Remove LangVer check from extended nameof binding (dotnet#62339) Fixed shared project file error (dotnet#62466) Handle new error codes Use MSBuid generated property for package path Exclude BCL libraries from Roslyn vsix Bump the integration test timeouts a bit Skip the balanced switch dispatch optimization for patterns on floating-point inputs (dotnet#62322) Test helpers shouldn't escape quotes by default (dotnet#62321) Reuse prior TableEntry values in the increment NodeTable builder if possible. (dotnet#62320) Install 3.1 runtime for SBOM tool Generate VS SBOM during official build. Minor refactoring in 'required' handling for override completion (dotnet#62422) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This drops us from 1.4GB of allocations:
to 1.15gb (18% drop)