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

SortedList allocates memory due to boxing of TKey #100406

Closed
PetrAltman opened this issue Mar 28, 2024 · 6 comments
Closed

SortedList allocates memory due to boxing of TKey #100406

PetrAltman opened this issue Mar 28, 2024 · 6 comments
Labels

Comments

@PetrAltman
Copy link

Description

SortedList<TKey, TValue> with struct TKey allocates memory due to boxing of TKey.

Configuration

Net 8,0

Analysis

Several methods in SortedList (e.g., Add, Remove, IndexOf) verify that the key is not null by invoking ArgumentNullException.ThrowIfNull(key);. ThrowIfNull expects an object parameter, causing TKey to be boxed when it is a struct type. This behavior should be avoided for struct types. It might be beneficial to add a generic overload to ArgumentNullException that does not allocate memory for struct types. This change could be useful as similar scenarios might occur in other generic classes within .NET.

@PetrAltman PetrAltman added the tenet-performance Performance related issue label Mar 28, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 28, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Mar 28, 2024
@SingleAccretion SingleAccretion added area-System.Collections and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 28, 2024
@stephentoub
Copy link
Member

Are you actually seeing that boxing happening in release with optimized code? The boxing associated with that ThrowIfNull gets optimized away by the JIT.

@PetrAltman
Copy link
Author

Oh, I didn't think of JIT could optimize that.

I had a naive benchmark:

var l = new SortedList<int, int>();

for (int it = 0; it < 100; it++)
{
    var a0 = GC.GetAllocatedBytesForCurrentThread();

    for (int i = 0; i < 1000; i++)
    {
        l.Remove(i);
    }

    var a1 = GC.GetAllocatedBytesForCurrentThread();

    Console.WriteLine(a1 - a0);
}

Where the number of iterations matters... This version returns on my machine:
24000
24000
24000
24000
24000
24000
24000
24000
24000
23520
0
0
0
0
0
0
0

It probably depends on when tiered JIT generates optimal code.
After switching TieredCompilation to false, it always returns 0.

@stephentoub
Copy link
Member

stephentoub commented Mar 28, 2024

Yes, in tier 0, almost all optimization is disabled (including but definitely not limited to boxing elimination). For ThrowIfNull, inlining needs to be enabled for the JIT to be able to see that the boxed object isn't actually needed, and inlining is one of the many optimizations disabled in tier 0, too.

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Mar 28, 2024
@MichalPetryka
Copy link
Contributor

The boxing associated with that ThrowIfNull gets optimized away by the JIT.

Is Mono able to remove it too or is it just with CoreCLR?

@huoyaoyuan
Copy link
Member

I'm concerned about how this would affect relatively cold methods. The impact of one particular method is limited by the threshold of Tier 0, but invoking many different methods may be impacted more.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2024
@EgorBo
Copy link
Member

EgorBo commented Jul 15, 2024

#104815 removed this boxing from Tier0 as well

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants