Inline properties during importation #96325

Suchiman · 2023-12-27T02:39:57Z

When we see a call to something in the shape of a property, we check if it does the same thing as an auto property and then pretend that we saw the LDFLD or STFLD instead of the CALL in the caller. This bypasses most of the inliner complexity and managed to improve the startup time of AvaloniaILSpy in my tests from 1483ms to 1373ms (7.5% improvement)

@EgorBo as discussed in discord

ghost · 2023-12-27T02:40:08Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

When we see a call to something in the shape of a property, we check if it does the same thing as an auto property and then pretend that we saw the LDFLD or STFLD instead of the CALL in the caller. This bypasses most of the inliner complexity and managed to improve the startup time of AvaloniaILSpy in my tests from 1483ms to 1373ms (7.5% improvement)

@EgorBo as discussed in discord

Author:	Suchiman
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

src/coreclr/jit/importercalls.cpp

jkotas · 2023-12-27T13:26:51Z

src/coreclr/jit/importer.cpp

@@ -8685,6 +8687,24 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                                  combine(combine(CORINFO_CALLINFO_ALLOWINSTPARAM, CORINFO_CALLINFO_SECURITYCHECKS),
                                          (opcode == CEE_CALLVIRT) ? CORINFO_CALLINFO_CALLVIRT : CORINFO_CALLINFO_NONE),
                                  &callInfo);
+
+                    if (callInfo.kind == CORINFO_CALL &&


Does this need to be skipped for debug code?

impTryFindField calls canInline which does this check

runtime/src/coreclr/vm/jitinterface.cpp

Lines 7879 to 7887 in 9fa5128

// If the callee wants debuggable code, don't allow it to be inlined

{

// Combining the next two lines, and eliminating jitDebuggerFlags, leads to bad codegen in x86 Release builds using Visual C++ 19.00.24215.1.

CORJIT_FLAGS jitDebuggerFlags = GetDebuggerCompileFlags(pCallee->GetModule(), CORJIT_FLAGS());

if (jitDebuggerFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE))

{

result = INLINE_NEVER;

szFailReason = "Inlinee is debuggable";

This check is for callee. Inlining needs to be disabled for debuggable caller too. It is typically done by checking opts.compDbgCode in the JIT.

jkotas · 2023-12-27T13:29:09Z

src/coreclr/jit/importercalls.cpp

@@ -32,6 +32,107 @@
 #pragma warning(disable : 21000) // Suppress PREFast warning about overly large function
 #endif

+bool Compiler::impTryFindField(CORINFO_METHOD_HANDLE methHnd, CORINFO_RESOLVED_TOKEN* pResolvedToken, OPCODE* opcode)
+{


This should issue beginInlining / reportInliningDecision callbacks.

This seems like we can end up issuing duplicate callbacks now (e.g. this one failing, then regular inlining succeeding). In general it seems unfortunate to end up with two separate inliners if we can avoid it, so some way of integrating this behavior into the existing inliner would be much preferred. For example, this new inlining should most likely affect the inline tree that gets reported by reportRichMappings too, and proper debug information should be created to refer to it.

src/coreclr/jit/importercalls.cpp

Suchiman · 2024-01-15T21:46:48Z

@MihuBot

EgorBo · 2024-01-31T23:29:10Z

Tested this PR on a large 1P service:

Before

IL Bytes Jitted (B)                                           29,854,910
Number of Methods Jitted                                         335,997

After

IL Bytes Jitted (B)                                           29,449,476
Number of Methods Jitted                                         300,224

AndyAyersMS · 2024-02-26T16:17:34Z

@Suchiman @EgorBo where are we at with this PR?

The approach seems promising, but we need to make sure we get all of the other effects of inlining handled properly.

Suchiman · 2024-02-27T20:12:01Z

@AndyAyersMS the PR is fully functional and correct enough to pass CI and improves startup speed as described in the initial post.
@EgorBo did offer to run crank to verify the gains but he didn't get around to that yet.

My hope was to get more numbers on this version (which will have the lowest possible overhead) and then investigate jakobs suggestion to integrate it more with the existing inliner, although i don't really see how that would work, and then get new numbers. Then we could compare this version, which might be a little bit ugly, with the architectural nicer version, to decide if the ugly'ness is worth it or not.

Egor did try inlining property sized methods in tier 0 code before but in his experiment, it regressed startup time (which motivated this PR in the first place), so i'm not hopeful that integrating this into the existing inliner will yield the same benefit. Egors experiment was based on a size treshold though, instead of precisely analyzing properties by IL as i do, which might move the bar but i would need to try.

EgorBo · 2024-03-04T12:46:18Z

@EgorBo did offer to run crank to verify the gains but he didn't get around to that yet.

I've just checked - it has no visible impact on TE. Mainly because TE benchmarks are too simplistic, there are only like 5k methods to jit. At the same time, this PR has nice impact on the 1P service I posted in #96325 (comment)

Egor did try inlining property sized methods in tier 0 code before but in his experiment, it regressed startup time (which motivated this PR in the first place), so i'm not hopeful that integrating this into the existing inliner will yield the same benefit. Egors experiment was based on a size treshold though, instead of precisely analyzing properties by IL as i do, which might move the bar but i would need to try.

Yes, mine was too generic, I expect yours to be more efficient in that scenario. The main issue with startup is that inlining may trigger unnecessary type load events etc, but, presumably, your case should not do that. So I'd just call the existing inline routine if an inlinee is precisely a getter/setter (so that we can avoid duplicating the logic and solve the debug info concerns Jakob raised). I'll collect new numbers if you do that

JulieLeeMSFT · 2024-04-01T17:58:20Z

avoid duplicating the logic and solve the debug info concerns Jakob raised).

@Suchiman, could you please take care of this?

JulieLeeMSFT · 2024-05-20T15:44:19Z

@EgorBo, PTAL at this community PR.

JulieLeeMSFT · 2024-05-20T16:42:54Z

@Suchiman, we see that you pushed some commits. Is it ready for code review or are you still working on it?

Suchiman · 2024-05-20T22:34:07Z

I'm still working on the feedback, but am currently on vacation

When we see a call to something in the shape of a property, we check if it does the same thing as an auto property and then pretend that we saw the LD(S)FLD or ST(S)FLD instead of the CALL in the caller - resolve tokens in the scope and with the generic context of the callee instead of the caller - call getFieldInfo with the scope of the callee to make visibility checks happy - make sure the callee is non virtual - respect basic no inlining instructions - getMethodInfo returns a success bool but might also throw

Suchiman · 2024-07-22T01:00:57Z

I've started work on the alternative approach here Suchiman@ad2c505 but i still need to play whack-a-mole with all the asserts that popped up from not having "MINOPTS" flags set in tier 0 compilation (OSR is specifically unhappy about that).

Is that going in the right direction @EgorBo ?

So I'd just call the existing inline routine if an inlinee is precisely a getter/setter (so that we can avoid duplicating the logic and solve the debug info concerns Jakob raised).

JulieLeeMSFT · 2024-08-19T18:32:25Z

@EgorBo, PTAL.

EgorBo · 2024-09-01T23:05:34Z

Taking a look

EgorBo · 2024-10-07T00:46:27Z

I've started work on the alternative approach here Suchiman@ad2c505 but i still need to play whack-a-mole with all the asserts that popped up from not having "MINOPTS" flags set in tier 0 compilation (OSR is specifically unhappy about that).

Is that going in the right direction @EgorBo ?

So I'd just call the existing inline routine if an inlinee is precisely a getter/setter (so that we can avoid duplicating the logic and solve the debug info concerns Jakob raised).

Sorry for the delayed response. We plan to eventually move inliner into an utility we can invoke from different phases, but we're not there yet. I agree with Jakob that the way it's written in this PR looks a bit fragile due to copy-paste from the general inliner making it more complicated to maintain and it should be somehow unified.
I've made my own attempt to enable Tier0 inlining in the past in here EgorBo@eab2ff7 the only thing that I enabled it for everything and introduced a new JIT-EE api to query IL size for the inlinee, presumably, you shouldn't need it here

Suchiman · 2024-11-04T01:14:11Z

due to copy-paste from the general inliner making it more complicated to maintain and it should be somehow unified.

I've been looking at unifying impTryFindField with impCheckCanInline for the past two days but they have less in common than apparent at first sight and use vastly different data structures, making that very non trivial.

I've made my own attempt to enable Tier0 inlining in the past in here EgorBo@eab2ff7

I'll try to give that a shot, otherwise, i'm running thin on options and motivation

ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 27, 2023

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 27, 2023

Suchiman force-pushed the alwaysInlineProps branch 2 times, most recently from afbe3ab to 0023bf1 Compare December 27, 2023 03:17

hez2010 reviewed Dec 27, 2023

View reviewed changes

src/coreclr/jit/importercalls.cpp Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Dec 27, 2023

Tracking issue for CI build timeouts #76454

Closed

jkotas reviewed Dec 27, 2023

View reviewed changes

EgorBo reviewed Dec 27, 2023

View reviewed changes

src/coreclr/jit/importercalls.cpp Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Jan 2, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

Suchiman force-pushed the alwaysInlineProps branch from ab9cfbb to 36c6db4 Compare January 2, 2024 23:51

This was referenced Jan 3, 2024

pip fails to build wheels for cffi (gcc not found) in ubuntu.1804.armarch.open using image alpine-3.15-helix-arm32v7 dotnet/dnceng#1423

Open

NRE in iOS.Device.Aot.Test when starting up xharness #96403

Closed

Suchiman force-pushed the alwaysInlineProps branch from d067dc1 to 3560ea4 Compare January 3, 2024 20:48

build-analysis bot mentioned this pull request Jan 11, 2024

Test failure - System.NullReferenceException in System.Threading.Lock.TryInitializeStatics #94728

Closed

Suchiman force-pushed the alwaysInlineProps branch 2 times, most recently from b99b37d to b46461b Compare January 15, 2024 10:42

MihuBot mentioned this pull request Jan 15, 2024

[X64] [Suchiman] Inline properties during importation MihuBot/runtime-utils#224

Open

JulieLeeMSFT assigned Suchiman Apr 1, 2024

JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 1, 2024

JulieLeeMSFT added this to the 9.0.0 milestone Apr 1, 2024

dotnet-policy-service bot added the no-recent-activity label Apr 15, 2024

Suchiman force-pushed the alwaysInlineProps branch from b46461b to c319a5b Compare April 27, 2024 17:37

dotnet-policy-service bot removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Apr 27, 2024

JulieLeeMSFT requested a review from EgorBo May 20, 2024 15:43

JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 24, 2024

dotnet-policy-service bot added the no-recent-activity label Jul 8, 2024

Suchiman force-pushed the alwaysInlineProps branch from c319a5b to 9afe6f6 Compare July 22, 2024 00:53

dotnet-policy-service bot removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Jul 22, 2024

AndyAyersMS modified the milestones: 9.0.0, Future Jul 29, 2024

Merge branch 'main' into alwaysInlineProps

1c7b26e

JulieLeeMSFT added the needs-author-action An issue or pull request that requires more info or actions from the author. label Oct 7, 2024

dotnet-policy-service bot added the no-recent-activity label Oct 21, 2024

dotnet-policy-service bot removed needs-author-action An issue or pull request that requires more info or actions from the author. no-recent-activity labels Nov 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline properties during importation #96325

Inline properties during importation #96325

Suchiman commented Dec 27, 2023

ghost commented Dec 27, 2023

jkotas Dec 27, 2023

Suchiman Dec 27, 2023

jkotas Dec 28, 2023

jkotas Dec 27, 2023

Suchiman Dec 27, 2023

jakobbotsch Jan 3, 2024

Suchiman commented Jan 15, 2024

EgorBo commented Jan 31, 2024

AndyAyersMS commented Feb 26, 2024

Suchiman commented Feb 27, 2024 •

edited

Loading

EgorBo commented Mar 4, 2024 •

edited

Loading

JulieLeeMSFT commented Apr 1, 2024

JulieLeeMSFT commented May 20, 2024

JulieLeeMSFT commented May 20, 2024

Suchiman commented May 20, 2024

Suchiman commented Jul 22, 2024

JulieLeeMSFT commented Aug 19, 2024

EgorBo commented Sep 1, 2024

EgorBo commented Oct 7, 2024 •

edited

Loading

Suchiman commented Nov 4, 2024

	// If the callee wants debuggable code, don't allow it to be inlined

	{
	// Combining the next two lines, and eliminating jitDebuggerFlags, leads to bad codegen in x86 Release builds using Visual C++ 19.00.24215.1.
	CORJIT_FLAGS jitDebuggerFlags = GetDebuggerCompileFlags(pCallee->GetModule(), CORJIT_FLAGS());
	if (jitDebuggerFlags.IsSet(CORJIT_FLAGS::CORJIT_FLAG_DEBUG_CODE))
	{
	result = INLINE_NEVER;
	szFailReason = "Inlinee is debuggable";

Inline properties during importation #96325

Are you sure you want to change the base?

Inline properties during importation #96325

Conversation

Suchiman commented Dec 27, 2023

ghost commented Dec 27, 2023

jkotas Dec 27, 2023

Choose a reason for hiding this comment

Suchiman Dec 27, 2023

Choose a reason for hiding this comment

jkotas Dec 28, 2023

Choose a reason for hiding this comment

jkotas Dec 27, 2023

Choose a reason for hiding this comment

Suchiman Dec 27, 2023

Choose a reason for hiding this comment

jakobbotsch Jan 3, 2024

Choose a reason for hiding this comment

Suchiman commented Jan 15, 2024

EgorBo commented Jan 31, 2024

AndyAyersMS commented Feb 26, 2024

Suchiman commented Feb 27, 2024 • edited Loading

EgorBo commented Mar 4, 2024 • edited Loading

JulieLeeMSFT commented Apr 1, 2024

JulieLeeMSFT commented May 20, 2024

JulieLeeMSFT commented May 20, 2024

Suchiman commented May 20, 2024

Suchiman commented Jul 22, 2024

JulieLeeMSFT commented Aug 19, 2024

EgorBo commented Sep 1, 2024

EgorBo commented Oct 7, 2024 • edited Loading

Suchiman commented Nov 4, 2024

Suchiman commented Feb 27, 2024 •

edited

Loading

EgorBo commented Mar 4, 2024 •

edited

Loading

EgorBo commented Oct 7, 2024 •

edited

Loading