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

Use a simpler pattern when working with stacks #73057

Merged
merged 41 commits into from
Apr 17, 2024

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 16, 2024
stack.Push(node);
try
Copy link
Member Author

Choose a reason for hiding this comment

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

view with whitespace off.

var workQueue = new Stack<ISymbol>();
workQueue.Push(_allSymbols);
using var _ = ArrayBuilder<ISymbol>.GetInstance(out var workQueue);
workQueue.AddRange(_allSymbols);
Copy link
Member Author

Choose a reason for hiding this comment

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

note: AddRange is fine here as _allSymbols is a set, so there's no concept of any sort of order to preserve when pushing onto the stack. Other AddRange calls in this PR work similarly.

if (current is INamespaceSymbol childNamespace)
{
stack.Push(childNamespace.GetMembers().AsEnumerable());
stack.AddRange(childNamespace.GetMembers().AsEnumerable());
Copy link
Member Author

Choose a reason for hiding this comment

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

no order of children. AddRange is fine.


var matches = FillWithIntervalsThatMatch(
start, length, testInterval,
ref builder, in introspector,
stopAfterFirst, candidates);

s_stackPool.ClearAndFree(candidates);
Copy link
Member Author

Choose a reason for hiding this comment

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

already done by the Releaser that a stack-pool uses.

@@ -124,16 +124,15 @@ private static IEnumerable<INamedTypeSymbol> GetTypesInFile(SemanticModel semant
using (Logger.LogBlock(FunctionId.NavigationBar_ItemService_GetTypesInFile_CSharp, cancellationToken))
{
var types = new HashSet<INamedTypeSymbol>();
var nodesToVisit = new Stack<SyntaxNode>();
using var _ = ArrayBuilder<SyntaxNode>.GetInstance(out var nodesToVisit);
Copy link
Member

Choose a reason for hiding this comment

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

curious - why are we changing only some usages of Stack to ArrayBuilder? Is it instances where we're not using a pooled Stack?

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Correct. If they had their own pool, I'm trusting them for now

Copy link
Member

Choose a reason for hiding this comment

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

Code not using a pool should, in general, not switch to using pools unless there is a measured problem. Each such change gives up a bit of memory safety and a bit of performance for no known (previously measured) gain.

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review April 17, 2024 01:00
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 17, 2024 01:00
@CyrusNajmabadi CyrusNajmabadi merged commit 7b18089 into dotnet:main Apr 17, 2024
25 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the stackPattern branch April 17, 2024 01:00
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Apr 17, 2024
@dibarbet dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE 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.

None yet

3 participants