Skip to content

Conversation

@ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented Jun 7, 2025

Fixes

Many allocations due to implicit object creation to capture function pointer + instance.

Context

Here's 72MB / 1.19M Func<IEnumerable<KeyValuePair<string, string>> objects being supposedly allocated by Utilities.CastItemsOnByOne:

image

In reality, the allocation is actually happening in the ItemData constructor:

public readonly struct ItemData
{
    private readonly Func<IEnumerable<KeyValuePair<string, string>>> _enumerateMetadata;

    public ItemData(string type, object value)
    {

        Type = type;
        Value = value;

        if (value is IItemData dt)
        {
            EvaluatedInclude = dt.EvaluatedInclude;
            _enumerateMetadata = dt.EnumerateMetadata;
        }
        else if (value is ITaskItem ti)
        {
            EvaluatedInclude = ti.ItemSpec;
            _enumerateMetadata = ti.EnumerateMetadata;
        }
        else
        {
            EvaluatedInclude = value.ToString() ?? string.Empty;
            _enumerateMetadata = () => [];
        }
    }
}

Referencing an instance method implicitly has to create an object since you need to store both a pointer to the target method that lives with the class definition + a pointer to the source instance to pass the method at runtime.

You can see this is exactly what the IL shows as well:

image

Changes Made

Since the struct already has a reference to the target in Value, we can directly call the function on-demand when needed without the allocation.

Copilot AI review requested due to automatic review settings June 7, 2025 05:55
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 removes the cached delegate in ItemData and inlines the metadata enumeration logic to eliminate allocations from capturing instance method pointers.

  • Removed the _enumerateMetadata field and its initialization in the constructor.
  • Updated EnumerateMetadata() to branch on Value at call time instead of storing a delegate.
  • Cleaned up an unused using System;.
Comments suppressed due to low confidence (2)

src/Framework/IItemData.cs:86

  • [nitpick] The multiple if/else branches could be simplified with a C# 12 switch expression for greater readability and conciseness, e.g.: return Value switch { IItemData dt => dt.EnumerateMetadata(), ITaskItem ti => ti.EnumerateMetadata(), _ => Array.Empty<KeyValuePair<string, string>>() };
public IEnumerable<KeyValuePair<string, string>> EnumerateMetadata()

src/Framework/IItemData.cs:88

  • New branching logic in EnumerateMetadata should be covered by unit tests for all three paths (IItemData, ITaskItem, and default) to prevent regressions.
if (Value is IItemData dt)

@SimaTian SimaTian merged commit 601428b into dotnet:main Jun 10, 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