Skip to content

Add AggressiveInlining to EnsureInitializedCore method to allow delegates stack alloc #122983

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.

Sign up for GitHub

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

Open
SergeyTeplyakov wants to merge 4 commits into
base: main
Choose a base branch
from
+10 −1
Open

Add AggressiveInlining to EnsureInitializedCore method to allow delegates stack alloc #122983

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8cb5625
Add AggressiveInlining to EnsureInitializedCore method to allow deleg…
SergeyTeplyakov Jan 7, 2026
2e72466
Merge branch 'main' into patch-1
SergeyTeplyakov Jan 7, 2026
5d8df04
Refactor EnsureInitializedCore to use Throw method
SergeyTeplyakov Jan 8, 2026
316d253
Update src/libraries/System.Private.CoreLib/src/System/Threading/Lazy…
SergeyTeplyakov Jan 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Threading/LazyInitializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;

namespace System.Threading
{
Expand Down Expand Up @@ -109,6 +110,7 @@ public static T EnsureInitialized<T>([NotNull] ref T? target, Func<T> valueFacto
/// <param name="target">The variable that need to be initialized</param>
/// <param name="valueFactory">The delegate that will be executed to initialize the target</param>
/// <returns>The initialized variable</returns>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static T EnsureInitializedCore<T>([NotNull] ref T? target, Func<T> valueFactory) where T : class
{
T value = valueFactory() ?? throw new InvalidOperationException(SR.Lazy_StaticInit_InvalidOperation);
Copy link
Member

@EgorBo EgorBo Jan 7, 2026

Choose a reason for hiding this comment

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

Moving throw new InvalidOperationException(SR.Lazy_StaticInit_InvalidOperation); to a throw-helper should also work, probably it's a better approach

Copy link
Author

@SergeyTeplyakov SergeyTeplyakov Jan 7, 2026

Choose a reason for hiding this comment

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

I've tried just moving it to a helper without AggressiveInlining, but it was not enough. The AggressiveInlining was still required in this case. And because of that, I've decided not to move the 'throw' into a helper.

Copy link
Member

@EgorBo EgorBo Jan 7, 2026

Choose a reason for hiding this comment

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

hm.. then it's fine as is, but the inliner needs some tuning apparently

Copy link
Member

@EgorBo EgorBo Jan 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

There are more EnsureInitializedCore overloads in this file, any reason why only this one needs AggressiveInlining?

Copy link
Author

@SergeyTeplyakov SergeyTeplyakov Jan 7, 2026

Choose a reason for hiding this comment

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

There are 2 more overloads that takes Func:

public static T EnsureInitialized<T>([NotNull] ref T? target, [NotNullIfNotNull(nameof(syncLock))] ref object? syncLock, Func<T> valueFactory) where T : class;

public static T EnsureInitialized<T>([AllowNull] ref T target, ref bool initialized, [NotNullIfNotNull(nameof(syncLock))] ref object? syncLock, Func<T> valueFactory);

I've tried adding AggressiveInlining to both of them (to EnsureInitializedCore methods), and here are the results:

| Method                                     | Runtime   | Mean      | Allocated |
|------------------------------------------- |---------- |----------:|----------:|
| EnsureInitialized_Old                      | .NET 10.0 |  5.873 ns |      88 B |
| EnsureInitialized_New                      | .NET 10.0 |  1.744 ns |      24 B |
| EnsureInitialized_SyncRoot_Old             | .NET 10.0 |  5.729 ns |      88 B |
| EnsureInitialized_SyncRoot_New             | .NET 10.0 |  5.692 ns |      88 B |
| EnsureInitialized_Initialized_SyncRoot_Old | .NET 10.0 | 18.475 ns |      24 B |
| EnsureInitialized_Initialized_SyncRoot_New | .NET 10.0 | 18.614 ns |      24 B |
| EnsureInitialized_Old                      | .NET 9.0  |  6.522 ns |      88 B |
| EnsureInitialized_New                      | .NET 9.0  |  6.561 ns |      88 B |
| EnsureInitialized_SyncRoot_Old             | .NET 9.0  |  6.566 ns |      88 B |
| EnsureInitialized_SyncRoot_New             | .NET 9.0  |  6.485 ns |      88 B |
| EnsureInitialized_Initialized_SyncRoot_Old | .NET 9.0  | 22.287 ns |      88 B |
| EnsureInitialized_Initialized_SyncRoot_New | .NET 9.0  | 19.889 ns |      88 B |

So in both cases the AggressiveInlining has no effect. For the first case, the JIT can't inline the method even with AggressiveInlining attribute so we still have 88B of allocations (for both the delegate and the closure), and for the second overload the JIT can inline the old version, so in both cases we have only the delegate allocation.

So I can add the attribute for consistency, but it seems that it has no effect currently for other 2 cases.

Copy link
Member

@stephentoub stephentoub Jan 8, 2026

Choose a reason for hiding this comment

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

Can you share what specifically you tried with regards to outlining the throw? With that this attr really shouldn't be necessary, and if it is, that suggests a broader inlining issue we should fix instead.

Copy link
Author

@SergeyTeplyakov SergeyTeplyakov Jan 8, 2026

Choose a reason for hiding this comment

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

Pushed another commit with the proposed change and trigerred the EgorBot to see the before and after results.
Running this version locally was not enough to make the Core method inlined by the JIT.

Expand Down
Loading