Allocate RuntimeType objects on Frozen Object Heap #75573
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsFollow up to #49576 Type Test() => typeof(int);Old codegen: ; Method Program:Test():System.Type:this
G_M50870_IG01:
4883EC28 sub rsp, 40
G_M50870_IG02:
48B968BB53F5FB7F0000 mov rcx, 0x7FFBF553BB68 ; System.Int32
E81D94505F call CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
90 nop
G_M50870_IG03:
4883C428 add rsp, 40
C3 ret
; Total bytes of code: 25New codegen: ; Method Program:Test():System.Type:this
G_M50870_IG01:
G_M50870_IG02:
48B8482A63DE29020000 mov rax, 0x229DE632A48 ; Type handle
G_M50870_IG03:
C3 ret
; Total bytes of code: 11Jit-diffs (-f --pmi): Size regressions look to be CSE-related, e.g. https://www.diffchecker.com/lDHsFUcP Notes:
|
EgorBo
commented
Sep 14, 2022
|
Seems to be passing CI tests (SPMI failures are due to JIT-EE change), @jkotas when you have time could you review the vm side? |
EgorBo
commented
Sep 14, 2022
jkotas
reviewed
Sep 14, 2022
jkotas
reviewed
Sep 14, 2022
jkotas
reviewed
Sep 14, 2022
Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
# Conflicts: # src/coreclr/inc/jiteeversionguid.h # src/coreclr/jit/valuenum.cpp
jkotas
reviewed
Sep 17, 2022
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
jkotas
reviewed
Sep 21, 2022
jakobbotsch
reviewed
Sep 21, 2022
jakobbotsch
reviewed
Sep 22, 2022
jakobbotsch
reviewed
Sep 22, 2022
| @@ -11161,7 +11181,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA | |||
| // | |||
| if (!tree->AsIndir()->IsVolatile()) | |||
| { | |||
| if (op1->IsIconHandle(GTF_ICON_STR_HDL)) | |||
| if (op1->IsIconHandle(GTF_ICON_OBJ_HDL)) | |||
There was a problem hiding this comment.
I guess we see this for cases like "" is string?
There was a problem hiding this comment.
I bet this can be removed since we now add these flags in morph anyway in case GT_IND: and the is volatile for string literal load seems to be useless - I'll check it in a follow up together with https://github.com/dotnet/runtime/pull/75573/files#r977367516
jakobbotsch
reviewed
Sep 22, 2022
| @@ -812,7 +812,7 @@ bool Compiler::optValnumCSE_Locate() | |||
| if (!enableConstCSE && | |||
| // Unconditionally allow these constant handles to be CSE'd | |||
| !tree->IsIconHandle(GTF_ICON_STATIC_HDL) && !tree->IsIconHandle(GTF_ICON_CLASS_HDL) && | |||
| !tree->IsIconHandle(GTF_ICON_STR_HDL)) | |||
| !tree->IsIconHandle(GTF_ICON_STR_HDL) && !tree->IsIconHandle(GTF_ICON_OBJ_HDL)) | |||
There was a problem hiding this comment.
Doesn't GTF_ICON_STR_HDL come with an invariant indirection on top? Do you know why we would CSE the constant but not the indir (now and also before)?
There was a problem hiding this comment.
@jakobbotsch normally it's expected that indirs on top of GTF_ICON_STR_HDL will be CSE'd, unlike raw constants they have higher weight and CSE should handle them on x64, I don't remember why exactly I put GTF_ICON_STR_HDL here in that other PR but AFAIR it affected diffs so I left it (initially I only wanted to handle GTF_ICON_CLASS_HDL) - I'll check diffs after this PR what will happen if I remove GTF_ICON_STR_HDL from here
There was a problem hiding this comment.
Potentially it was because we weren't setting GTF_IND_INVARIANT for the string handles in R2R? That was fixed in #70986, so might be this is unnecessary today.
There was a problem hiding this comment.
Ah, good point, will check (in a follow up)
jakobbotsch
approved these changes
Sep 22, 2022
|
Looks like this can be merged now, two Pri0 failures are from #75952, SPMI ones are due to JIT-EE guid change (we still don't know how to avoid triggering those when both JIT and JIT-EE guid are changed in YML) I plan to look into the profiler/diag issues raised in the other PR for FOH, and test this and the string literals on OSS (and, hopefully, some 1st parties) projects to get a better understanding how many objects make it to FOH. Then we might look into making |
How do you plan to do that? I do not think it is feasible without AOT step (or without doing expensive work when JITing static constructors - I do not expect we would want to do that). |
|
Can we do it for static field boxes (the ones created for non-primitives)? Today, JIT needs to create ADD byref
IND ref
CNS <object slot addr>
CNS 8for all such accesses, I believe. We could potentially create direct pointer to the boxed contents. |
|
Ah ok, yes - these boxes can be allocated on the frozen heap. |
|
Good point about the static boxes, regarding other types of objects I am not sure either Perhaps, for e.g. static readonly string str = ...;we could insert |
14 tasks
jkotas
added a commit
to jkotas/runtime
that referenced
this pull request
Sep 27, 2022
)" This reverts commit 1f1231c.
jkotas
added a commit
that referenced
this pull request
Sep 27, 2022
|
Around ~100 improvements dotnet/perf-autofiling-issues#8700, a few regressions and some are noise |
EgorBo
added a commit
that referenced
this pull request
Oct 6, 2022
…en objects" (#76649) This PR reverts #76235 with a few manual modifications: The main issue why the initial PRs (#75573 and #76135) were reverted has just been resolved via #76251: GC could collect some associated (with frozen objects) objects as unreachable, e.g. it could collect a SyncBlock, WeakReferences and Dependent handles associated with frozen objects which could (e.g. for a short period of time) be indeed unreachable but return back to life after. Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up to #49576
This PR allocates Type objects on the recently-added Frozen Segments so we can bake direct objects' addresses in JIT and avoid helper calls, e.g. for:
Old codegen:
New codegen:
Jit-diffs (-f --pmi):
Size regressions look to be CSE-related, e.g. https://www.diffchecker.com/eOr1LIyf
E.g. previously
call CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPEcould share its const argument with other places where we only need type handle (like in this diff)Notes:
In JIT I do the optimization in morph because a lot of other optimizations here and there (mostly in importer) rely on
typeof()being a helper call so it's simpler for now, will try to perform a sort of clean up later - current change is minimalistic and doesn't produce regressions.Closes #49429