Remove lazy initialized string literals

[EgorBo]

It seems it's not worth it to avoid allocations on the frozen heap - the lazy string helper has a lock inside (Crst in GetStringLiteral) and it badly impacts performance for throw Exception(<string literal>) code (as was reported internally) especially under contention.

    static void Foo(bool cond)
    {
        throw new Exception("Hello");
    }

; Method Program:Foo(bool) (FullOpts)
       push     rbx
       sub      rsp, 32
       mov      rcx, 0x7FFBD79E2E10      ; System.Exception
       call     CORINFO_HELP_NEWSFAST
       mov      rbx, rax
-      mov      ecx, 1
-      mov      rdx, 0x7FFBD7FE0000
-      call     [CORINFO_HELP_STRCNS]
-      mov      rdx, rax
       mov      rcx, rbx
+      mov      rdx, 0x28922695108      ; 'Hello'
       call     [System.Exception:.ctor(System.String):this]
       mov      rcx, rbx
       call     CORINFO_HELP_THROW
       int3     
-; Total bytes of code: 65
+; Total bytes of code: 51

[EgorBo]

cc @dotnet/jit-contrib @jkotas opinions?

(I guess I can also remove getLazyStringLiteralHelper API)

[dotnet-policy-service]

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

[jkotas]

I expect that this change will regress working set since we are going to allocate memory for these strings eagerly. Do you have an estimate for what this regression is going to be for a typical real-world app?

the lazy string helper has a lock inside (Crst in GetStringLiteral)

This can be made lock-free if we cared.

I guess I can also remove getLazyStringLiteralHelper API

Yes, and the CORINFO_HELP_STRCNS helper. (Note that this optimization is not enabled in crossgen2 - #48580.)

[kg]

Code LGTM.

Does this meaningfully increase app startup time or time spent in the JIT itself? It seems like it would, but maybe not to an extent that matters.

[EgorBo]

This can be made lock-free if we cared.

Sure, but still will be a bit less efficient than just a frozen object (it also unlocks other constant folding optimizations that we might run even in cold blocks). Probably still makes sense to optimize the VM if someone screams at us for poor String.Intern performance, e.g. https://sergeyteplyakov.github.io/Blog/benchmarking/2023/12/10/Intern_or_Not_Intern.html:

* String interning in non-native AOT is very slow and can drastically affect your application performance.

Does this meaningfully increase app startup time or time spent in the JIT itself? It seems like it would, but maybe not to an extent that matters.

The main concern here is an increased working set as the JIT now has to allocate string objects even for cold blocks (it might eventually be fixed by a 'partial compilation' feature if it ever lands for optimized code) - it should be relatively quick using FrozenHeapManager, but it's very unlikely we're talking about a huge impact. It seems that when string interpolation syntax is used - it's not impacted as all string-interpolation logic is happening outside of BBJ_THROW.

[jkotas]

still will be a bit less efficient than just a frozen object

Depends on what you are measuring. We typically avoid regressing non-exceptional paths to make exception throwing faster. Frozen string allocation is doing the opposite.

[jkotas]

(I am ok with taking this change as a simplification that comes with a small regression.)

[am11]

The main concern here is an increased working set as the JIT now has to allocate string objects even for cold blocks

Is the increase in working set a one-time cost, or does it grow with each string allocated in frozen heap?

[EgorBo]

The main concern here is an increased working set as the JIT now has to allocate string objects even for cold blocks

Is the increase in working set a one-time cost, or does it grow with each string allocated in frozen heap?

it's one time, all string literals are registered in a global interning table. It's actually doesn't matter if it's frozen or not (frozen saves a bit of space by not occupying pinned handles).

We typically avoid regressing non-exceptional paths to make exception throwing faster.

I'll analyze the impact on a big app, but I'm pretty sure the impact will be negligible simply because all localizeable (SR.*) or interned strings aren't impacted, so only raw string literals in BBJ_THROW blocks are, as you can see from the removed code, we never actually enabled the lazy helpers for all cold blocks.

[tannergooding]

the lazy string helper has a lock inside (Crst in GetStringLiteral)

This can be made lock-free if we cared.

This seems like something that would be beneficial to look into. Even with this PR we won't always use the FOH (such as for unloadable assemblies) and locking the stringLiteralMap when we can just resolve an existing entry seems "bad". I'd think it would be better to only lock when adding the entry, if possible, so its a "one time cost" instead.

[teo-tsirpanis]

Would it make sense to not intern string literals when the string is known to be used in a throw statement, and always construct a new string?

[jkotas]

Would it make sense to not intern string literals when the string is known to be used in a throw statement, and always construct a new string?

It would be a breaking change.

[EgorBo]

@jkotas so I analyzed a few local apps and I was not able to detect any visible working set size regression because it was within noise, but I did noticed extra objects in the frozen heap, e.g. OrchardCMS:

Main:
| MT | Total Count | Total Size | Name

7ffcbd81e1e0 8,035   475,120 System.String

PR:

7ffc997ae1e0 8,502   521,680 System.String

so extra 45kb of working set (after 120sec of bombardier load) for an app that occupies ~1GB of working set (so +0.005%)

& $bombardierPath -d 120s -c 16 -t 2s --insecure -l --fasthttp --header "Accept: text/plain,text/html;q=0.9,application/xhtml+xml;q=0.9,application/xml;q=0.8,*/*;q=0.7" --header "Connection: keep-alive" "$url/about"
Start-Sleep -Seconds 3
$dump = "heap-${serverProcess.Id}.dmp"
dotnet-dump collect -p $serverProcess.Id -o $dump | Out-Null
dotnet-dump analyze $dump -c "dumpheap -type System.String -stat -gen foh"

The Foh numbers were stable unlike all other generations.

[Copilot]

Pull request overview

This PR removes the lazy string literal initialization optimization to improve performance by eliminating lock contention in the CORINFO_HELP_STRCNS helper. The optimization was previously used in specific scenarios like throw blocks, but the lock overhead outweighed the benefits of avoiding allocations on the frozen heap.

• Removes the CORINFO_HELP_STRCNS JIT helper and all associated infrastructure
• Eliminates the getLazyStringLiteralHelper JIT interface method
• Simplifies exception throwing code by removing the lazy initialization path in fgMorphConst

Reviewed changes

Copilot reviewed 31 out of 31 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/coreclr/vm/qcallentrypoints.cpp	Removes QCall entry point for String_StrCns
src/coreclr/vm/jitinterface.cpp	Removes getLazyStringLiteralHelper implementation and ReadyToRun helper mapping
src/coreclr/vm/corelib.h	Removes STRCNS method definition macro
src/coreclr/vm/appdomainnative.hpp	Removes String_StrCns function declaration
src/coreclr/vm/appdomainnative.cpp	Removes String_StrCns QCall implementation
src/coreclr/tools/superpmi/superpmi/icorjitinfo.cpp	Removes superpmi wrapper for getLazyStringLiteralHelper
src/coreclr/tools/superpmi/superpmi-shim-simple/icorjitinfo_generated.cpp	Removes shim-simple interceptor
src/coreclr/tools/superpmi/superpmi-shim-counter/icorjitinfo_generated.cpp	Removes shim-counter interceptor
src/coreclr/tools/superpmi/superpmi-shim-collector/icorjitinfo.cpp	Removes shim-collector interceptor
src/coreclr/tools/superpmi/superpmi-shared/methodcontext.h	Comments out packet enum and removes method declarations
src/coreclr/tools/superpmi/superpmi-shared/methodcontext.cpp	Removes recording/replay methods for getLazyStringLiteralHelper
src/coreclr/tools/superpmi/superpmi-shared/lwmlist.h	Removes GetLazyStringLiteralHelper lightweight map entry
src/coreclr/tools/aot/jitinterface/jitinterface_generated.h	Removes callback function pointer and wrapper method
src/coreclr/tools/Common/JitInterface/ThunkGenerator/ThunkInput.txt	Removes getLazyStringLiteralHelper from thunk input
src/coreclr/tools/Common/JitInterface/CorInfoImpl_generated.cs	Reduces callback array from 178 to 177 and shifts indices down after removal
src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs	Removes getLazyStringLiteralHelper stub implementation
src/coreclr/tools/Common/JitInterface/CorInfoHelpFunc.cs	Removes CORINFO_HELP_STRCNS enum value
src/coreclr/jit/valuenumfuncs.h	Removes VNF_LazyStrCns value number function
src/coreclr/jit/valuenum.cpp	Removes CORINFO_HELP_STRCNS to VNF_LazyStrCns mapping
src/coreclr/jit/utils.cpp	Removes CORINFO_HELP_STRCNS helper properties
src/coreclr/jit/morph.cpp	Removes entire lazy string construction logic in fgMorphConst
src/coreclr/jit/compiler.hpp	Removes CORINFO_HELP_STRCNS from IsSharedStaticHelper check
src/coreclr/jit/ICorJitInfo_wrapper_generated.hpp	Removes wrapper method
src/coreclr/jit/ICorJitInfo_names_generated.h	Removes API name entry
src/coreclr/inc/readytorun.h	Marks READYTORUN_HELPER_GetString as no longer supported in v17.0
src/coreclr/inc/jithelpers.h	Removes CORINFO_HELP_STRCNS dynamic helper entry
src/coreclr/inc/jiteeversionguid.h	Updates JIT-EE version GUID to reflect interface change
src/coreclr/inc/icorjitinfoimpl_generated.h	Removes method declaration
src/coreclr/inc/corinfo.h	Removes CORINFO_HELP_STRCNS enum and getLazyStringLiteralHelper interface method
src/coreclr/System.Private.CoreLib/src/System/String.CoreCLR.cs	Removes StrCns and StrCnsInternal managed methods
docs/design/coreclr/botr/readytorun-format.md	Updates documentation to mark GetString as unused since v17.0