-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
CG2 Add lazy string literal loading support #47183
Conversation
nattress
commented
Jan 19, 2021
- Currently all string literals are loaded as method load pre-code fixups.
- Match CG1 implementation and allow the JIT to use lazy string literal resolution so cold code strings are lazy loaded.
* Currently all string literals are loaded as method load pre-code fixups. * Allow the JIT to use lazy string literal resolution so cold code strings are lazy loaded.
cc @dotnet/crossgen-contrib |
@@ -799,6 +799,9 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum) | |||
case CorInfoHelpFunc.CORINFO_HELP_JIT_REVERSE_PINVOKE_EXIT: | |||
id = ReadyToRunHelper.ReversePInvokeExit; | |||
break; | |||
case CorInfoHelpFunc.CORINFO_HELP_STRCNS_CURRENT_MODULE: | |||
id = ReadyToRunHelper.GetString; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think it is as simple as this. There needs to be a code to add the current module argument for the helper (it is what ZapLazyHelperThunk
does in crossgen1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I thought the CORINFO_HELP_STRCNS_CURRENT_MODULE
meant we didn't need that. I'll look into adding that lazy thunk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that ImportThunk already has a Lazy variant that I think was supposed to cater for lazy strings, among others. Having said that, it hasn't been tested until now so there's a high chance of it being broken in some way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Tomas - I corrected things to call the lazy thunk and fixed an x64 bug. I'll kick off a CG2 run to make sure we're okay on other architectures.
* Emit a lazy thunk through to the `ReadyToRunHelper.GetString` helper which provides the current module indirection cell to the helper * Fix `X64Emitter.EmitMov` to set the address mode. The existing call sequence was causing an AV due to the DS register only having the lower 32 bits set.
@@ -819,6 +822,11 @@ private ISymbolNode GetHelperFtnUncached(CorInfoHelpFunc ftnNum) | |||
return _compilation.NodeFactory.GetReadyToRunHelperCell(id); | |||
} | |||
|
|||
private CorInfoHelpFunc getLazyStringLiteralHelper(CORINFO_MODULE_STRUCT_* handle) | |||
{ | |||
return CorInfoHelpFunc.CORINFO_HELP_STRCNS_CURRENT_MODULE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right for strings from other modules? Crossgen1 version of this method has a check for current module in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that's a fragile ngen scenario. getLazyStringLiteralHelper
gets called from the below handling code in the JIT. Returning CORINFO_HELP_STRCNS
causes the JIT to call embedModuleHandle
which is not supported in R2R.
runtime/src/coreclr/jit/morph.cpp
Lines 9287 to 9301 in 1390941
CorInfoHelpFunc helper = info.compCompHnd->getLazyStringLiteralHelper(tree->AsStrCon()->gtScpHnd); | |
if (helper != CORINFO_HELP_UNDEF) | |
{ | |
// For un-important blocks, we want to construct the string lazily | |
GenTreeCall::Use* args; | |
if (helper == CORINFO_HELP_STRCNS_CURRENT_MODULE) | |
{ | |
args = gtNewCallArgs(gtNewIconNode(RidFromToken(tree->AsStrCon()->gtSconCPX), TYP_INT)); | |
} | |
else | |
{ | |
args = gtNewCallArgs(gtNewIconNode(RidFromToken(tree->AsStrCon()->gtSconCPX), TYP_INT), | |
gtNewIconEmbScpHndNode(tree->AsStrCon()->gtScpHnd)); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this all work well for large version bubble or composite mode cases?
src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/CorInfoImpl.ReadyToRun.cs
Outdated
Show resolved
Hide resolved
…foImpl.ReadyToRun.cs Co-authored-by: Jan Kotas <jkotas@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you! Could you please address JanK's concerns by running a private composite Crossgen2 lab run on the change?
I'm abandoning this change. I initially made it to reduce file size; however the generated code is larger than the savings from removing a method pre-code fixup. In their current form lazy string literals are only used for string literals in catch blocks so the optimization is a marginal one anyway. |
Lazy string literals are used in throw blocks. They are startup time and working set optimization. We may want to measure how much startup time and working set we are giving up by not implementing them. |