Skip to content

Conversation

@ttstanley
Copy link
Contributor

@ttstanley ttstanley commented May 30, 2025

Fixes : Allocation issue.

Context

Looking at a trace of allocations. It was shown that some of the allocations were coming from closures. This pr addresses the closures found.

Changes Made

  • Removed lazy from exclude tester function since it was not needed since lifetime of lazy object was within the method itself.
  • switched from linq clause for add range to manually adding items, because the linq version caused a closure from a method it did not have context with.

Testing

Used ILSpy to verify that the closures disappeared. (DisplayClass represents a closure)
Before
image

Afterwards
image

Notes

Copilot AI review requested due to automatic review settings May 30, 2025 21:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR eliminates closures and LINQ allocations by switching from a lazy Func<string, bool> to an eagerly initialized delegate and by replacing .Where(...) calls with manual loops.

  • Changed excludeTester from Lazy<Func<string,bool>> to Func<string,bool> and moved its initialization into the processing loop.
  • Replaced LINQ-based filtering (AddRange(...Where(...))) with explicit foreach and conditional Add.
  • Updated ValueFragment handling to call the tester directly.
Comments suppressed due to low confidence (1)

src/Build/Evaluation/LazyItemEvaluator.IncludeOperation.cs:55

  • The variable excludePatternsForGlobs is declared but never used; consider removing it to reduce dead code.
ISet<string>? excludePatternsForGlobs = null;

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Before you fix the bug, can you please try to add a unit test that fails instead

@rainersigwald rainersigwald requested a review from SimaTian June 2, 2025 17:12
@ttstanley
Copy link
Contributor Author

Before you fix the bug, can you please try to add a unit test that fails instead

@rainersigwald I'm not sure I follow on what scenario to fail on the unit test would be. Can you explain a bit more?

@rainersigwald
Copy link
Member

Oh sorry, that comment was left over from your first revision that failed on the second-phase build; you've fixed it already. Ignore it!

@rainersigwald rainersigwald merged commit cf273b1 into dotnet:main Jun 6, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants