CG2 Add lazy string literal loading support#47183
CG2 Add lazy string literal loading support#47183nattress wants to merge 5 commits intodotnet:masterfrom
Conversation
nattress
commented
Jan 19, 2021
nattress
commented
- 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 |
| id = ReadyToRunHelper.ReversePInvokeExit; | ||
| break; | ||
| case CorInfoHelpFunc.CORINFO_HELP_STRCNS_CURRENT_MODULE: | ||
| id = ReadyToRunHelper.GetString; |
There was a problem hiding this comment.
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.
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.
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.
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.
|
|
||
| private CorInfoHelpFunc getLazyStringLiteralHelper(CORINFO_MODULE_STRUCT_* handle) | ||
| { | ||
| return CorInfoHelpFunc.CORINFO_HELP_STRCNS_CURRENT_MODULE; |
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
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 <[email protected]>
trylek
left a comment
trylek
left a comment
There was a problem hiding this comment.
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. |
ghost
locked as resolved and limited conversation to collaborators
3 participants
