Multicorejit unification by gbalykov · Pull Request #48326 · dotnet/runtime

gbalykov · 2021-02-16T07:54:16Z

This PR unifies non-generic/shared-generic methods with generic methods in multicorejit, allowing to keep the original jit order. This is required as part of #45748 change (specifically, #45748 (comment)), goal of which is to enable background type preloading using multicorejit.

Generic methods are stored in profile using their binary signature and non-generics methods are stored using method tokens. However, for non-generic methods record size has double (8 bytes now), which allowed to fit additional data. Thus, there's no limit on method token now (previously there was METHODINDEX_MASK). Also, limit for module index has increased from 512 (MAX_MODULES) to 4096. Additional info can be stored for each method like JIT_BY_APP_THREAD_TAG or NO_JIT_TAG (NO_JIT_TAG will be added in further changes).

Additionally, NDirect methods are removed, because their current implementation leads to asserts and exceptions. Also, few checks for number of cpus are configurable with env variables now, thus, allowing to use multicorejit on arm with cpu hotplug.

cc @alpencolt

mangod9 · 2021-03-29T16:39:12Z

@gbalykov assume this is ready to merge? @noahfalk is this something you could be able to review?

noahfalk · 2021-03-29T22:22:04Z

It may take me a couple days, but yes I'll take a look : ) Also @kouvel might be a good person to give it a look. Tiered compilation has some overlap with multicore JIT.

noahfalk · 2021-04-01T09:40:23Z

Spent some time tonight learning about this code. Not done yet but haven't forgotten : )

gbalykov · 2021-04-01T09:48:02Z

@mangod9 yes, this is ready for review and merge

noahfalk

This looks good to me. Most of the comment are some stylistic suggestions but I do think removing the requirement for multiple processors should be evaluated by others.

@mangod9 - Ultimately if a bug was uncovered I wouldn't be the dev who owned fixing it. Whoever that dev would be its probably good if they get a chance to look at this before it is merged : )

noahfalk · 2021-04-01T08:13:50Z

Nit:
Suggested change
// 5. Simple module name stored
// 6. Simple module name stored

noahfalk · 2021-04-02T23:59:01Z

What enforces that this length assertion will be true?

It was an assertion before but the length limit was higher. I don't know how long signature lengths can get, this should probably be a real check and skip the method in that case.

noahfalk · 2021-04-03T00:02:04Z

Earlier the code enforced that sigSize < MAX_SIGNATURE_LENGTH, but does anything enforce that AlignUp(sigSize + 8, 4) < MAX_SIGNATURE_LENGTH?

noahfalk · 2021-04-03T00:03:37Z

Nit:
Suggested change
DWORD dwSize = ((DWORD)(dataSize + sizeof(DWORD) - 1) / sizeof(DWORD)) * sizeof(DWORD);
DWORD dwSize = AlignUp(dataSize, sizeof(DWORD));

noahfalk · 2021-04-03T00:32:29Z

Removing this should get some scrutiny around performance. Even if removing it makes some scenarios better, probably there was a reason this was here. I don't think I'd be the right person to assess that part of the change though.
cc @mangod9 @kouvel @brianrob

Agreed @noahfalk. @gbalykov, can you share more details about this change? My expectation is that if there is only one processor, then playing back a Multi-Core JIT profile is likely to impact start-up negatively.

On arm devices cpus might be enabled dynamically (cpu hotplug). Thus, during coreclr_initialize there might be only one enabled cpu, yet there'll be more when there's enough work to do

Thanks @gbalykov. My first thought is that if we're going to remove this check, we're going to want to limit the removal to Linux arm builds. It looks like per https://community.arm.com/developer/tools-software/oss-platforms/w/docs/529/cpuidle-hotplug, hotplug is only available on Linux.
My next thought is that I'm wanting to understand more about the hotplug behavior. Multi-Core JIT looks at the CPU count once when it gets invoked, and that's it. It sounds like we might need to modify Multi-Core JIT to be hotplug aware (if that's possible) so that we don't needlessly do aggressive assembly loading and jitting on the critical path if we want to support the hotplug scenario on arm. Is it possible to be hotplug aware?

Another option may be to make it configurable for now via clrconfigvalues.h and keep the default as it is currently. On CPU-limited environments where there is actually only one processor available, the background thread could otherwise steal a fair bit of CPU time from foreground work.

Another option may be to make it configurable for now via clrconfigvalues.h
I've added this solution instead of check removal. By default 2 cpus will still be needed

noahfalk · 2021-04-03T01:10:30Z

Nit: The rest of this code uses PascalCasing for method names so I would align to that style
Suggested change
staticboolisMethod(unsigneddata)
staticboolIsMethod(unsigneddata)

noahfalk · 2021-04-03T01:51:47Z

Suggested change
MulticoreJitTrace(("ModuleRecord(%u) start module load",
MulticoreJitTrace(("ModuleDependency(%u) start module load",

noahfalk · 2021-04-03T01:53:00Z

Suggested change
MulticoreJitTrace(("ModuleRecord(%d) end module load, hr=%x",
MulticoreJitTrace(("ModuleDependency(%d) end module load, hr=%x",

noahfalk · 2021-04-03T02:02:43Z

Nit: if this is never false we may as well eliminate the if statement

This actually would be part of the next PR, that's why initially I left it here even if it was dead code. Removed from this PR

noahfalk · 2021-04-03T02:06:00Z

This block is dead code I think?

kouvel · 2021-04-12T14:41:43Z

I'll take a look as well

kouvel

Generally looks good to me, some comments below. Thanks!

kouvel · 2021-04-16T18:18:50Z

Is it necessary to support this many modules? A large array of this many elements is allocated so if it's not necessary maybe something like 0x400 or 0x1000 would be reasonable. It looks like MAX_MODULES - 1 is also used as a mask, could update to use a separate constant MODULE_MASK that is specific to the packing format.

I agree that 0х1000 would be enough

kouvel · 2021-04-16T18:21:25Z

In the usage it looks like the max allowed signature length is actually 0xffff. To make it more clear I suggest changing the value to 0xffff (and usage to match), and maybe rename to SIGNATURE_LENGTH_MASK since it is also used as a mask.

kouvel · 2021-04-16T18:58:16Z

It should be fairly fast to extract the record type with a shift and to check the ID, it doesn't seem to be worth making these powers of 2 just for that purpose (though I could have missed something)

Yes, what I wanted was to have simple bitfield check in RecorderInfo::IsMethod instead of shift and equality comparison. I agree that this is not critical at all, fixed

kouvel · 2021-04-16T19:11:29Z

I found the uses of the fields data1, data2, and ptr are difficult to read/understand (the uses are spread out too much), I suggest encapsulating away the data fields by making them private, and use constructors and instance methods to operate on the data/packing/unpacking, would be more clear in usage and knowledge of the data formats would hopefully be limited to methods in the class. Using a union of structs as @noahfalk suggested would also be helpful.

kouvel · 2021-04-16T19:26:21Z

m_ModuleList could be allocated separately too for consistency, since that could also be a modestly sized array. My preference would be to keep that and m_JitInfoArray as inline arrays as before (MulticoreJitRecorder is new'ed anyway, and the behavior in some error conditions would be more consistent with before), though perhaps the inline arrays could be moved to be the last fields.

kouvel · 2021-04-19T07:04:09Z

Looks like this could fail if count happened to be 0 and hr == COR_E_BADIMAGEFORMAT from above. Probably need a SUCCEEDED(hr) here and in the if expression below.

kouvel · 2021-04-19T07:14:20Z

I think this should either be & (MAX_SIGNATURE_LENGTH - 1) (if its value is 0x10000), or & MAX_SIGNATURE_LENGTH (if its value is changed to 0xffff as I suggested in another comment). In any case it appears the length is not used, so it could just be removed.

Thanks for noticing! I've fixed this typo, but forgot to update it here

kouvel · 2021-04-19T07:24:14Z

Looks like this is not used and could be removed

This'll become used in next PR. Removed from this PR

kouvel · 2021-04-19T07:24:53Z

Same comment as above regarding & MAX_SIGNATURE_LENGTH or & (MAX_SIGNATURE_LENGTH - 1), could be removed

kouvel · 2021-04-19T07:28:00Z

Another option may be to make it configurable for now via clrconfigvalues.h and keep the default as it is currently. On CPU-limited environments where there is actually only one processor available, the background thread could otherwise steal a fair bit of CPU time from foreground work.

gbalykov · 2021-05-04T17:22:30Z

@noahfalk @kouvel Thanks for reviews! I've updated PR according to your notes.

I've also measured mcj profile size with different modes and here are the results. For all cases there were no difference in performance between different modes. So, with current version of PR profile size increases on just 26-31%, instead of 100% increase on initial version of this PR.

1. x64 debug, console helloworld, no ni.dll:

Mode	Profile size, bytes
before this PR	5872
initial PR	11292
initial PR with removed overall record size (`597c7c9`)	9820
initial PR with removed overall record size and tokens for non-generic methods (`b5cb5ae`)	7412

2. tizen armel release, console helloworld, no ni.dll:

Mode	Profile size, bytes
before this PR	3944
initial PR	7716
initial PR with removed overall record size (`597c7c9`)	6720
initial PR with removed overall record size and tokens for non-generic methods (`b5cb5ae`)	5060

3. tizen armel release, crossgen2 for crossgen2.dll, no ni.dll:

./corerun ./crossgen2/crossgen2.dll -r:`pwd`/*.dll -r:`pwd`/crossgen2/*.dll -O --parallelism=1 -o:/tmp/1.ni.dll `pwd`/crossgen2/crossgen2.dll

Mode	Profile size, bytes
before this PR	54648
initial PR	113028
initial PR with removed overall record size (`597c7c9`)	95128
initial PR with removed overall record size and tokens for non-generic methods (`b5cb5ae`)	71648

Current multicorejit implementation in master has multiple flaws with NDirect methods: - exception might be thrown inside GetStubForInteropMethod at some point for NDirect method, which will kill background thread, thus, reducing effectiveness of multicorejit (for example, occurs when multicorejit is used with crossgen2) - some NDirect methods can lead to asserts during load inside GetStubForInteropMethod (for example, EvpMdCtxDestroy (0x6000044 token) from System.Security.Cryptography.Algorithms.dll)

…igure minimum allowed number of cpus for MultiCoreJit. On arm with cpu hotplug it should be set to 1.

This change reduces mcj profile size

kouvel

Mostly looks good to me, just one main comment and a couple of minor things. Thanks for sharing the data, that looks good to me as well.

kouvel · 2021-05-07T00:51:55Z

+for (LONG i = 0 ; i < m_JitInfoCount; i++)
 {
- for (LONG i = 0 ; i < m_GenericInfoCount; i++)
+ SigBuilder sigBuilder;


This could be moved into the if block for generic methods

kouvel · 2021-05-07T01:39:12Z

 HRESULT hr = S_OK;

- // Preprocessing Generic Methods
+ // Preprocessing Methods


The additional comments below are no longer valid, could be removed

kouvel · 2021-05-07T02:16:05Z

+ }
+ else if (rcdTyp == MULTICOREJIT_GENERICMETHOD_RECORD_ID)
+ {
+ unsigned signatureLength = * (const unsigned short *) (((const unsigned *) pBuffer) + 1);


It seems to be the intention in this part to check if there is enough space in the remaining buffer before decoding. From the loop condition we know that there is a DWORD worth of space but this is reading past that, so should check the remaining buffer length before dereferencing.

kouvel

LGTM, thanks!

kouvel · 2021-05-14T00:15:43Z

Failure looks unrelated, #52527

gbalykov · 2021-05-14T08:55:23Z

Thanks!

mangod9 · 2021-05-14T15:46:45Z

Thanks for the contribution @gbalykov and @kouvel / @noahfalk for reviewing.

* Fix assertion failure / crash in multi-core JIT - When the recorder times out it doesn't actually stop profiling, but writes out the profile - The app may later stop profiling, and then it tries to write the profile again - PR #48326 fairly expected that the profile is only written once (some state is mutated) - The non-timeout stop-profile path was also not stopping the timer - Fix for #53014 in main

* Add background type preloading based on multicorejit This is a second part of #48326 change, which enables handling of methods loaded from r2r images. Background thread of multicorejit now not only jits methods but also loads methods from R2R images. This allows to load types in background thread. This is required as part of #45748 change (specifically, #45748 (comment)), goal of which is to enable background type preloading using multicorejit.

ghost added the area-TieredCompilation-coreclr label Feb 16, 2021

gbalykov force-pushed the multicorejit-unification branch 4 times, most recently from 9d4d725 to 956aa05Compare February 16, 2021 10:30

Base automatically changed from master to mainMarch 1, 2021 09:07

noahfalk reviewed Apr 3, 2021
View reviewed changes

gbalykov mentioned this pull request Apr 5, 2021
Fix NDirect methods in multicorejit #50651
Closed

kouvel reviewed Apr 19, 2021
View reviewed changes

kouvel added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) and removed NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) labels Apr 19, 2021

gbalykov force-pushed the multicorejit-unification branch from 956aa05 to b5cb5aeCompare May 4, 2021 17:07

gbalykov force-pushed the multicorejit-unification branch from b5cb5ae to 9107365Compare May 4, 2021 17:36

gbalykov added 2 commits May 4, 2021 22:07

Unify simple and generic methods in multicorejit
ed6862b

gbalykov force-pushed the multicorejit-unification branch from 9107365 to 3be92a6Compare May 4, 2021 19:07

Add MultiCoreJitMinNumCpus env variable with default value =2 to conf…
26e631a
…igure minimum allowed number of cpus for MultiCoreJit. On arm with cpu hotplug it should be set to 1.

gbalykov force-pushed the multicorejit-unification branch from 3be92a6 to 339a561Compare May 4, 2021 20:45

gbalykov added 3 commits May 5, 2021 11:38

Review fixes
0081de5

Do not save overall record length for methods
cd2820f
This change reduces mcj profile size

Store non-generic methods using token instead of signatures
4a3c5d2
This change reduces mcj profile size

gbalykov force-pushed the multicorejit-unification branch from 339a561 to 4a3c5d2Compare May 5, 2021 08:38

runfoappBot mentioned this pull request May 5, 2021
Failure to publish artifacts #52334
Closed

kouvel reviewed May 7, 2021
View reviewed changes

Additional fixes
f8af3e6

kouvel approved these changes May 7, 2021
View reviewed changes

gbalykov mentioned this pull request May 11, 2021
Add background type preloading based on multicorejit #52595
Merged

kouvel merged commit 886a02c into dotnet:mainMay 14, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

jkotas mentioned this pull request Jun 1, 2021
[.NET 6 Preview 5] dnSpy stopped working while decompiling assembly #53014
Closed

This was referenced Jun 2, 2021
Fix assertion failure / crash in multi-core JIT #53573
Merged
[6.0 preview 5] Fix assertion failure / crash in multi-core JIT #53574
Closed

ghost locked as resolved and limited conversation to collaborators Jun 19, 2021

	// 5. Simple module name stored
	// 6. Simple module name stored

	DWORD dwSize = ((DWORD)(dataSize + sizeof(DWORD) - 1) / sizeof(DWORD)) * sizeof(DWORD);
	DWORD dwSize = AlignUp(dataSize, sizeof(DWORD));

	staticboolisMethod(unsigneddata)
	staticboolIsMethod(unsigneddata)

	MulticoreJitTrace(("ModuleRecord(%u) start module load",
	MulticoreJitTrace(("ModuleDependency(%u) start module load",

	MulticoreJitTrace(("ModuleRecord(%d) end module load, hr=%x",
	MulticoreJitTrace(("ModuleDependency(%d) end module load, hr=%x",

Uh oh!

Conversation

gbalykov commented Feb 16, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mangod9 commented Mar 29, 2021

Uh oh!

noahfalk commented Mar 29, 2021

Uh oh!

noahfalk commented Apr 1, 2021

Uh oh!

gbalykov commented Apr 1, 2021

Uh oh!

noahfalk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kouvel commented Apr 12, 2021

Uh oh!

kouvel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gbalykov commented Feb 16, 2021•
edited
Loading

kouvel left a comment •
edited
Loading