Skip to content

Perf: Reimplement Lookup.Scope tables without ItemDictionary#12320

Merged
YuliiaKovalova merged 4 commits intodotnet:mainfrom
ccastanedaucf:dev/chcasta/perf-lookupdictionaries
Sep 30, 2025
Merged

Perf: Reimplement Lookup.Scope tables without ItemDictionary#12320
YuliiaKovalova merged 4 commits intodotnet:mainfrom
ccastanedaucf:dev/chcasta/perf-lookupdictionaries

Conversation

@ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented Aug 12, 2025

Fixes

Reduces a significant amount of CPU and allocations in Lookup related to ItemDictionary overhead.

Before:
image

After (-2.8s of CPU - most of the remaining time is related to metadata Modifies, PropertyDictionary, and the outer scope which still uses ItemDictionary)
image

Allocations are harder to 1:1 granularly, but you can see on the left a total ~870MB reduction.

Before:
image

After (-870MB):
image

Context

Lookup uses a stack of Scope objects which each track multiple optional tables of ProjectItemInstance (implemented via ItemDictionary).

internal class Scope
{
      internal ItemDictionary<ProjectItemInstance> Items { get; set; }
      internal ItemDictionary<ProjectItemInstance> Adds { get; set; }
      internal ItemDictionary<ProjectItemInstance> Removes { get; set; }

      // More properties not relevant to this PR
}

For the context of the PR, here's a quick overview of how this currently works:

  • When a scope is "popped", each table except for Items is merged into the next scope. After this point, the popped scope is effectively discarded (see MergeScopeIntoNotLastScope()).
  • When the outer scope is reached, the set of item adds/removes + any metadata modifications are applied to the base scope's Items (see MergeScopeIntoLastScope()). Unlike other scopes, this base table is externally provided at the construction of the first Lookup.
    • The outer scope only exists to track the base collections passed into the first scope - as such, its adds/removes/modification tables are always empty.
  • GetItems() returns a non-destructive merge down the scope stack, stopping if either the outer scope is reached, or a scope is found with its Items table set.
    • Note that this currently creates an additional ItemDictionary, even though we only return a single item type list.
    • Intermediate Items tables generally only contain one item type with items, with others containing "empty markers". These are used to mask the base item dictionary, primarily in ItemBucket where each bucket (and therefore scope) holds a single type.

Currently, each of these intermediate tables are implemented via ItemDictionary, which internally uses a Dictionary<string, LinkedList> to represent item lists and a Dictionary<ProjectItemInstance, LinkedList> to provide O(1) access to indices. Empty markers are implemented by creating empty entries. Every method also takes a lock, since ItemDictionary is designed for use in other areas of MSBuild - whereas Lookup is only used in single-threaded contexts.

internal sealed class ItemDictionary<T> : IItemDictionary<T>
    where T : class, IKeyed, IItem
{
    private readonly Dictionary<string, LinkedList<T>> _itemLists;
    private readonly Dictionary<T, LinkedListNode<T>> _nodes;
}

This all adds overhead that can be avoided by replacing ItemDicitonary with basic collections for all inner scopes.

Changes Made

  • Introduce a lighter-weight ItemDictionarySlim to replace ItemDictionary inside of Lookup, while still providing similar helper methods.
internal class ItemDictionarySlim : IEnumerable<KeyValuePair<string, List<ProjectItemInstance>>>
{
    private readonly Dictionary<string, List<ProjectItemInstance>> _itemLists;

   // helper methods...
}
  • Implement empty markers as a reuseable frozen set - ItemTypesToTruncateAtThisScope
internal class Scope
{
      internal FrozenSet<string> ItemTypesToTruncateAtThisScope { get; set; }

      // Other properties...
}
  • Track removes as a simple list concatenation, instead of deduplicating at each scope
  • Avoid a host of intermediate allocations during GetItems

Copilot AI review requested due to automatic review settings August 12, 2025 04:00
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 optimizes performance by replacing the ItemDictionary implementation in Lookup.Scope with a lighter-weight ItemDictionarySlim, reducing CPU usage and memory allocations. The changes eliminate locking overhead and simplify data structures used for tracking item additions, removals, and empty markers within lookup scopes.

  • Introduces ItemDictionarySlim to replace ItemDictionary for scope-specific item tracking
  • Replaces empty markers with a frozen set-based truncation approach using ItemTypesToTruncateAtThisScope
  • Optimizes item merging and removal logic to avoid duplicate checking and intermediate allocations

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Lookup.cs Core performance optimization replacing ItemDictionary with ItemDictionarySlim and implementing new truncation mechanism
ItemDictionary.cs Removes unused methods (AddRange, Replace, AddEmptyMarker, HasEmptyMarker) to simplify API
IItemDictionary.cs Updates interface by removing unused method signatures
ImmutableItemDictionary.cs Removes overridden implementations of deleted interface methods
ItemBucket.cs Updates to use new truncation API instead of empty markers
BatchingEngine.cs Changes to use FrozenSet for item names in bucket creation
TargetEntry.cs Adds explicit truncation calls for incremental build scenarios
TargetUpToDateChecker.cs Replaces AddEmptyMarker calls with ImportItemsOfType
ItemGroupIntrinsicTask.cs Updates RemoveItems call signature to include item type parameter
Test files Updates test code to work with new APIs and data structures

@surayya-MS surayya-MS self-requested a review September 29, 2025 08:54
Copy link
Member

@surayya-MS surayya-MS left a comment

Choose a reason for hiding this comment

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

Looks good to me!
@ccastanedaucf let me know if you would like to merge it as is or address comments

@YuliiaKovalova YuliiaKovalova merged commit 9a3cb6b into dotnet:main Sep 30, 2025
9 checks passed
YuliiaKovalova pushed a commit that referenced this pull request Oct 27, 2025
**NOTE:**
- ~First couple commits are duplicated from #12320 - it removes some
`ItemDictionary` APIs that simplify this refactor by eliminating most
use cases for the O(1) lookup, so just ignore them for now.~ Force
pushed the rebased version.

### Context

After #12320 , the O(1) `LinkedListNode` lookup in `ItemDictionary` is
only used for removes.

If removes are batched by the same item type (see new
`RemoveItemsByItemType()`, the cost to build a `HashSet` lookup
on-demand is trivial compared to the current approach of maintaining an
additional O(1) lookup table with every dictionary entry - especially
given that removes are only used in a few scenarios.

### Perf

Working set memory at end of build *(-17% and nearly the entire Large
Object Heap)*:
**Before**
<img width="360" height="178" alt="image"
src="https://github.com/user-attachments/assets/b8cb5ffd-cfdf-47d2-bc56-ca19b7f6e60e"
/>

**After**
<img width="386" height="180" alt="image"
src="https://github.com/user-attachments/assets/522ddb3b-f39f-464c-ad1d-551c32f4f42f"
/>

Total allocations *(-300MB, finally no longer double digits!)*:
**Before**
<img width="1202" height="470" alt="image"
src="https://github.com/user-attachments/assets/f048bf29-ea0c-4768-abd0-b2e8d6570f3e"
/>

**After**
<img width="1202" height="406" alt="image"
src="https://github.com/user-attachments/assets/691983f4-8d04-44f7-a5cb-78ba12ec4e7f"
/>


This one also has a pretty significant drop in total GC time relative to
allocations, I'm guessing due to the difference in LOH? I compared 4
profiles here to make sure this wasn't noise.

**Before**
<img width="1178" height="460" alt="image"
src="https://github.com/user-attachments/assets/ee20aabd-a27e-4002-9dfa-7ca70b3ecb2c"
/>

**After**
<img width="1226" height="422" alt="image"
src="https://github.com/user-attachments/assets/bb883650-fd76-4e45-b544-d2df68770d40"
/>

This also further reduces CPU when merging down to the base Scope. Much
of the remaining cost here was related to updating the additional
dictionary, and this is also the only location where batch removes
occur.

**Before**
<img width="913" height="169" alt="image"
src="https://github.com/user-attachments/assets/a39c91a5-6c6f-4d17-b0eb-9b549f98e475"
/>

**After**
<img width="920" height="221" alt="image"
src="https://github.com/user-attachments/assets/42496d81-b22f-45a3-bfaa-458c19e7f186"
/>
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.

4 participants