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 3 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
5 changes: 4 additions & 1 deletion 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 @@ -111,10 +112,12 @@ public static T EnsureInitialized<T>([NotNull] ref T? target, Func<T> valueFacto
/// <returns>The initialized variable</returns>
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);
T value = valueFactory() ?? Throw();
Interlocked.CompareExchange(ref target, value, null!);
Debug.Assert(target != null);
return target;

static T Throw() => throw new InvalidOperationException(SR.Lazy_StaticInit_InvalidOperation);
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.

Suggested change
T value = valueFactory() ?? Throw();
Interlocked.CompareExchange(ref target, value, null!);
Debug.Assert(target != null);
return target;
static T Throw() => throw new InvalidOperationException(SR.Lazy_StaticInit_InvalidOperation);
T? value = valueFactory();
if (value is null)
{
Throw();
}
Interlocked.CompareExchange(ref target, value, null!);
Debug.Assert(target != null);
return target;
[DoesNotReturn]
static void Throw() => throw new InvalidOperationException(SR.Lazy_StaticInit_InvalidOperation);

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.

Happy to take this one. I've tried this version as well, and it was not inlined. My idea was that the generic helper was problematic here, so I've changed it to a void, but then changed it to generic in this PR.

Once the EgorBot will finish running my version, I'll accept this change to run it again.

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.

@EgorBo, do you know why this wouldn't be inlined?

Copy link
Member

@EgorBo EgorBo Jan 8, 2026

Choose a reason for hiding this comment

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

@SergeyTeplyakov @stephentoub this highlights a problem we have in JIT + PGO. We do make inliner more aggressive in hot blocks by making it less aggressive in cold blocks. Here

public static T EnsureInitialized<T>([NotNull] ref T? target, Func<T> valueFactory) where T : class
{
    var read = Volatile.Read(ref target!);
    if (read != null)
    {
        return read;
    }
    return EnsureInitializedCore(ref target, valueFactory);
}

EnsureInitializedCore is basically a cold block, so inliner decides to give up on it. In most cases this is the right thing to do, but it does mess with the escape analysis at times. We had many other places similar to this with the same issue, until we fix it (by making inliner more sophisticated or implement inter-procedural analysis, the AggressiveInlining is the only way to go 😞). cc @AndyAyersMS

you can validate my theory by setting target to point to null so the cold path is always executed (I already checked locally).

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.

Oh, wait, this is EnsureInitiskized_Core_. We don't want that to be able inlined; it's very explicitly separated out as we don't want it cluttering up call sites.

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.

It was clear from the get go that this is a balancing act here, but since it seems that there is a plan to make it inlineable in the future, maybe this trade is worth it?

My line of reasoning is the following. We have 3 cases here:

  • A method is used a lot and a delegate can be stack-allocated. Probably a win.
  • A method is not used a lot. The delegate won't be stack-allocated. A minor regression. But since the method is not used a lot, its not that important.
  • A method is used a lot, but the delegate can't be stack-allocated due to other reasons.

So the trade-off is between cases 1 and 3. I have only an intiutive sense that the benefits will outweight the cost. Especially given the fact that the JIT might get better and will start inlining the method (if the method is too big, JIT won't be able to inline it even with the hint).

Copy link
Contributor

@MichalPetryka MichalPetryka Jan 8, 2026

Choose a reason for hiding this comment

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

Oh, wait, this is EnsureInitiskized_Core_. We don't want that to be able inlined; it's very explicitly separated out as we don't want it cluttering up call sites.

@EgorBo Would it be possible to teach the JIT to move the delegate allocation into the cold if instead?

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.

A method is used a lot, but the delegate can't be stack-allocated due to other reasons.

A super common case is a delegate that's a cached singleton because it's static. By forcing inlining, we're making the whole thing larger, which then negatively impacts the possibility of its callers being inlined / makes all such consumers larger... standard negative impact of inlining things that shouldn't be inlined.

Copy link
Member

@AndyAyersMS AndyAyersMS Jan 8, 2026

Choose a reason for hiding this comment

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

@EgorBo Would it be possible to teach the JIT to move the delegate allocation into the cold if instead?

Maybe.

We should be able to see that the delegate is partially dead and (with PGO) likely to be dead. If so we could sink its allocation and construction like you say. We haven't really looked into sinking partially dead code. It might be doable, though proving it is safe and moving all the right pieces might be hard.

Or if we had partial escape analysis... we'd see that the delegate was unlikely to escape and so we'd stack allocate it initially and then move it to the heap if needed.

Copy link
Author

@SergeyTeplyakov SergeyTeplyakov Jan 9, 2026

Choose a reason for hiding this comment

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

@stephentoub I did have concerns that this change might not be useful given all the trade offs and it seems that from your perspective its not worth taking it. Right?
If we don't see the path forward, I'm ok abandoning it.

}

/// <summary>
Expand Down
Loading