On Stack Replacement Next Steps

Possible next steps now that #32969 is merged, in rough order of priority.

Issues and fixes after OSR was enabled

Performance Regressions

Other ideas: enhancements or optimizations

cc @dotnet/jit-contrib

category:cq
theme:osr
skill-level:expert
cost:extra-large

And if you want to try this out (x64 only):

COMPlus_TC_QuickJitForLoops=1
COMPlus_TC_OnStackReplacement=1

By default a transition requires ~10,000 executions of a patchpoint. Unlike tiered compilation, transitions are currently synchronous, so there's no lag/delay once this threshold is reached.

To enable "aggressive OSR" -- where an OSR method is created the first time a patchpoint is encountered, also set

COMPlus_TC_OnStackReplacement_InitialCounter=1
COMPlus_OSR_HitLimit=1

To see OSR log messages from the runtime, also add

COMPlus_LogEnable=1
COMPlus_LogFacility=0x00400000
COMPlus_LogLevel=4
COMPlus_LogToConsole=1

Design sketch for better supporting patchpoints within `try` regions

As noted above and elsewhere (#35687) the jit has a strong assumption that control can only enter a try region by branching to the first block in the try. If there's an OSR patchpoint within the try we currently branch directly from the prolog to the middle of the try, breaking this assumption.

The idea here is to add extra control flow to the starts of any try region that encloses the patchpoint, to conditionally transfer control from outside the try to the appropriate point. So all control flow still enters the try at the first block. If we're coming from method entry, we jump to the next enclosing try or the patchpoint as appropriate.

For example, suppose we have the following (Sn are arbitrary blocks of code):

S0;
try {
    S1;
    try {
         S2;
         for (...) {}
    }
}

The for will inspire a patchpoint that is nested within two try regions. We would transform this as follows:

__firstCall = 1;
goto pt1;
S0; 
pt1:
try {
    if (_firstCall == 1) goto pt2;
    S1;
    pt2:
    try {
        if (_firstCall == 1) goto pp;    
        S2;
        pp:
        _firstCall = 0;
        for (...)
    }
}

Because of the conditional control flow we'll end up importing some Sn blocks that we currently skip. If it turns out the various Sn are unreachable (which should be common) we should be able to optimize them away.

Since we can detect the try-nesting of a patchpoint when we're asked to create an OSR method we won't need extra info passed in the patchpoint records from Tier0. We just apply this transformation automatically if we see the patchpoint is within a try.

Note patchpoints within handers are still problematic, largely because of funclets. So if we see loops in catches or finallies we might still want to have the method bypass Tier0. These should be rare.

Started working on the try region fix noted just above. Seems like we ought to wait until after the importer has done its "transitive closure" to add the OSR inspired try region "step blocks" -- if we add them before importation then we will make all the code within tries reachable and this may be hard to undo.

If after importation an enclosing try's entry block is unreachable then the step block can unconditionally transfer to the enclosed try entry and/or actual entry point.

Still checking if it's ok for a try entry to have multiple preds or if we actually expect there to be a unique fall through pred.

Here's an example from the tailrecursetry test case with my current changes main..FixMidTryPatchpoint.

This is adding the new flow pre-importation; as noted above that might not be the best approach overall. Seems like perhaps we should defer this until fgRemoveEmptyBlocks to avoid wrangling with the importer, and then we can introduce conditional flow only if needed.

;; before (BB10->BB03 is a "mid try entry")

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd                 weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB10 [0009]  1                             1       [???..???)-> BB03 (always)                     i internal 
BB01 [0000]  1                             1       [000..002)                                     i bwd bwd-target 
BB02 [0001]  1  0                          1       [002..006)-> BB04 (always) T0      try {       keep i try bwd 
BB03 [0002]  2  0                          1       [006..00E)                 T0                  i bwd bwd-target 
BB04 [0003]  2  0                          1       [00E..012)-> BB03 ( cond ) T0                  i bwd 
BB05 [0004]  1  0                          1       [012..014)-> BB07 (always) T0      }           i bwd 
BB06 [0005]  1     0                       1       [014..017)-> BB07 ( cret )    H0   catch { }   keep i bwd 
BB07 [0006]  2                             1       [017..01B)-> BB09 ( cond )                     i bwd 
BB08 [0007]  1                             1       [01B..01D)        (return)                     i 
BB09 [0008]  1                             1       [01D..02D)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------

;; after (BB12 -> BB03 steps in via try entry BB02)

-----------------------------------------------------------------------------------------------------------------------------------------
BBnum BBid ref try hnd                 weight    lp [IL range]     [jump]      [EH region]         [flags]
-----------------------------------------------------------------------------------------------------------------------------------------
BB12 [0011]  1                             1       [???..???)-> BB02 (always)                     i internal 
BB01 [0000]  1                             1       [000..002)                                     i bwd bwd-target 
BB02 [0001]  2  0                          1       [???..???)-> BB03 ( cond ) T0      try {       keep i internal try bwd 
BB11 [0010]  1  0                          1       [002..006)-> BB04 (always) T0                  keep i bwd 
BB03 [0002]  2  0                          1       [???..???)                 T0                  i internal bwd bwd-target 
BB10 [0009]  1  0                          1       [006..00E)                 T0                  i bwd 
BB04 [0003]  2  0                          1       [00E..012)-> BB03 ( cond ) T0                  i bwd 
BB05 [0004]  1  0                          1       [012..014)-> BB07 (always) T0      }           i bwd 
BB06 [0005]  1     0                       1       [014..017)-> BB07 ( cret )    H0   catch { }   keep i bwd 
BB07 [0006]  2                             1       [017..01B)-> BB09 ( cond )                     i bwd 
BB08 [0007]  1                             1       [01B..01D)        (return)                     i 
BB09 [0008]  1                             1       [01D..02D)        (return)                     i 
-----------------------------------------------------------------------------------------------------------------------------------------

JIT: better support for patchpoints in try regions

JIT: better support for patchpoints in try regions (#59784)

If there's an OSR method that has multiple patchpoints and multiple active threads we can trigger an "OSR storm" of sorts

Compiling  157 Program::fannkuch, IL size = 708, hash=0x19770f83 Tier0
Compiling  158 Microsoft.FSharp.Collections.ArrayModule::Create, IL size = 105, hash=0xb95c3591 Tier0
Compiling  159 Program::direct@15, IL size = 183, hash=0x7e81a925 Tier0
Compiling  160 Program::fannkuch, IL size = 708, hash=0x19770f83 Tier1-OSR @0x150
Compiling  160 Program::fannkuch, IL size = 708, hash=0x19770f83 Tier1-OSR @0x119
Compiling  160 Program::fannkuch, IL size = 708, hash=0x19770f83 Tier1-OSR @0x137
Compiling  160 Program::fannkuch, IL size = 708, hash=0x19770f83 Tier1-OSR @0x21f
Compiling  160 Program::fannkuch, IL size = 708, hash=0x19770f83 Tier1-OSR @0x25d
Compiling  160 Program::fannkuch, IL size = 708, hash=0x19770f83 Tier1-OSR @0xe5

This seems kind of unfortunate, but not sure there's any easy fix.

One could imagine, say, deferring OSR generation for a method if there's already an OSR method being built at any patchpoint in the method, with the hope that other threads eventually hit and can use the one OSR method being created. But there's no guarantee that any of these threads will ever hit other patchpoints.

(1) Been looking into what would be required to enable QJFL=1 via OSR. There are still some methods that OSR can't handle, and some loops within methods that OSR can't handle.

So the proposal is that with QJFL=1 and OSR=1 (which would become the default on x64, say) and provided the JIT could guarantee escape the Tier0 method via OSR, we'd jit at Tier0. If the JIT can't guarantee this then we instead optimize. So a much larger set of methods would jit at Tier0.

We could reserve QJFL=2 say to mean "always jit at Tier0" regardless, if we wanted such capability.

Bailing out of Tier0+OSR because of the "method" checks this is easy enough as it can use the same pattern we already use for QJFL=0. But for the per-loop failures, it's not as clear how to handle the fallback -- as we need to import to detect the problem.

If these cases are rare enough, it could work like we do for minopts fallback, failing the Tier0 jit request with OSR set; then the driver could request a new jit with OSR not set.

Or we could try and cleanly architect a jit reset that bails from mid-import back to pre-import somehow, but there's a lot of ambient jit state around....

(2) For OSR + {stack,loc}alloc, we'd need to have the jit and gc info support the idea of having 3 pointers into the frame. One would point at the original method's frame base (to access the OSR state), one at the OSR method's frame base (to access any state it introduces, eg spilled temps), and one to refer to the outgoing args area. The separation between these 3 would vary depending on how much allocation was done by the base method before triggering OSR, and how much is done by the OSR method.

Unfortunately the idea that there are at most two frame pointers (FP and SP) is baked in everywhere, including the GC encoding.

Fixing this is straightforward in principle, but also a fair amount of work. And while there are potentially other uses for three frame pointers (eg using the 3rd to reduce the need for large frame-relative offsets), at least initially this support would be exclusive to OSR, and within OSR exclusive to that subset of methods using localloc. So not faring well in terms of cost/benefit.

So thinking now that the right move is to initially exclude these methods from OSR by detecting this as one of the cases we can't handle, and bailing to optimized code (like we do currently for QJFL=0).

Have an interim comparison report showing OSR vs MAIN. The top dozen or so regressions are the ssl: true tests, these all should be "fixed" by the next BDN update in the performance repo: dotnet/performance#2323

Overall this report shows 395 regressions and 591 improvements. The vast majority of these are noise. Drew is going to get me an updated comparison report that takes history into account; when that shows up I'll post an update.

In the meantime looking through what's there for actual regressions, Benchstone.BenchF.NewtR.Test is slower with OSR. This benchmark runs 4 iterations per, so ends up executing OSR for about 1/3 of its workload iterations. The OSR codegen is similar but the inner loop has a couple of extra loads because of an OSR exposed local.

I actually get somewhat inconsistent results locally. And not clear yet why we can't hoist/cse this load. Need to look more closely what's going on.

;; FullOpt/Tier1 inner loop


                            align    [0 bytes for IG04]

G_M46661_IG04:              ;; offset=0030H
       C5CB59D6             vmulsd   xmm2, xmm6, xmm6
       C5FB101DE4000000     vmovsd   xmm3, qword ptr [reloc @RWD00]
       C5E35CD2             vsubsd   xmm2, xmm3, xmm2
       C5EB59D6             vmulsd   xmm2, xmm2, xmm6
       C5FB5CD2             vsubsd   xmm2, xmm0, xmm2
       C5E8541DF0000000     vandps   xmm3, xmm2, qword ptr [reloc @RWD32]
       C5F92ECB             vucomisd xmm1, xmm3
       7746                 ja       SHORT G_M46661_IG06
       C5CB591DF2000000     vmulsd   xmm3, xmm6, qword ptr [reloc @RWD48]
       C5E359DE             vmulsd   xmm3, xmm3, xmm6
       C5E35C1DB6000000     vsubsd   xmm3, xmm3, qword ptr [reloc @RWD00]
       C5D857E4             vxorps   xmm4, xmm4
       C5F92EDC             vucomisd xmm3, xmm4
       7A02                 jp       SHORT G_M46661_IG05
       7421                 je       SHORT G_M46661_IG06
						;; bbWeight=16    PerfScore 597.33
G_M46661_IG05:              ;; offset=0076H
       C5EB5ED3             vdivsd   xmm2, xmm2, xmm3
       C5CB5CF2             vsubsd   xmm6, xmm6, xmm2
       C5E85415BA000000     vandps   xmm2, xmm2, qword ptr [reloc @RWD32]
       C5F92ECA             vucomisd xmm1, xmm2
       7707                 ja       SHORT G_M46661_IG06
       FFC0                 inc      eax
       83F80A               cmp      eax, 10
       7E9D                 jle      SHORT G_M46661_IG04

;; OSR inner loop


                            align    [0 bytes for IG04]

G_M46661_IG04:              ;; offset=0066H
       C5FB109424A0000000   vmovsd   xmm2, qword ptr [rsp+A0H]       // extra
       C5EB59DA             vmulsd   xmm3, xmm2, xmm2
       C5FB1025C5010000     vmovsd   xmm4, qword ptr [reloc @RWD16]
       C5DB5CDB             vsubsd   xmm3, xmm4, xmm3
       C5E359D2             vmulsd   xmm2, xmm3, xmm2
       C5FB5CD2             vsubsd   xmm2, xmm0, xmm2
       C5E8541DC1010000     vandps   xmm3, xmm2, qword ptr [reloc @RWD32]
       C5F92ECB             vucomisd xmm1, xmm3
       7768                 ja       SHORT G_M46661_IG06
       C5FB109C24A0000000   vmovsd   xmm3, qword ptr [rsp+A0H]       // extra
       C5E35925BA010000     vmulsd   xmm4, xmm3, qword ptr [reloc @RWD48]
       C5DB59DB             vmulsd   xmm3, xmm4, xmm3
       C5E35C1D8E010000     vsubsd   xmm3, xmm3, qword ptr [reloc @RWD16]
       C5D857E4             vxorps   xmm4, xmm4
       C5F92EDC             vucomisd xmm3, xmm4
       7A02                 jp       SHORT G_M46661_IG05
       7439                 je       SHORT G_M46661_IG06
						;; bbWeight=16    PerfScore 693.33
G_M46661_IG05:              ;; offset=00BEH
       C5EB5ED3             vdivsd   xmm2, xmm2, xmm3
       C5FB109C24A0000000   vmovsd   xmm3, qword ptr [rsp+A0H]       // extra
       C5E35CDA             vsubsd   xmm3, xmm3, xmm2
       C5FB119C24A0000000   vmovsd   qword ptr [rsp+A0H], xmm3
       C5E8541570010000     vandps   xmm2, xmm2, qword ptr [reloc @RWD32]
       C5F92ECA             vucomisd xmm1, xmm2
       770B                 ja       SHORT G_M46661_IG06
       FFC3                 inc      ebx
       83FB0A               cmp      ebx, 10
       0F8E75FFFFFF         jle      G_M46661_IG04

;; OSR iterations

// AfterActualRun
WorkloadResult   1: 4 op, 369514200.00 ns, 92.3786 ms/op
WorkloadResult   2: 4 op, 361280500.00 ns, 90.3201 ms/op
WorkloadResult   3: 4 op, 357723200.00 ns, 89.4308 ms/op
WorkloadResult   4: 4 op, 361092200.00 ns, 90.2730 ms/op
WorkloadResult   5: 4 op, 355732600.00 ns, 88.9331 ms/op
WorkloadResult   6: 4 op, 352204300.00 ns, 88.0511 ms/op
WorkloadResult   7: 4 op, 299682100.00 ns, 74.9205 ms/op
WorkloadResult   8: 4 op, 282106800.00 ns, 70.5267 ms/op
WorkloadResult   9: 4 op, 283662600.00 ns, 70.9156 ms/op
WorkloadResult  10: 4 op, 283777300.00 ns, 70.9443 ms/op
WorkloadResult  11: 4 op, 284411100.00 ns, 71.1028 ms/op
WorkloadResult  12: 4 op, 288849500.00 ns, 72.2124 ms/op
WorkloadResult  13: 4 op, 281122300.00 ns, 70.2806 ms/op
WorkloadResult  14: 4 op, 281138100.00 ns, 70.2845 ms/op
WorkloadResult  15: 4 op, 282122600.00 ns, 70.5306 ms/op
WorkloadResult  16: 4 op, 286633300.00 ns, 71.6583 ms/op
WorkloadResult  17: 4 op, 286545500.00 ns, 71.6364 ms/op
WorkloadResult  18: 4 op, 282719600.00 ns, 70.6799 ms/op
WorkloadResult  19: 4 op, 284713200.00 ns, 71.1783 ms/op
WorkloadResult  20: 4 op, 285504900.00 ns, 71.3762 ms/op

In the Tier0 method, x0 is address exposed, so the OSR method has to reload it from memory each time.

The exact impact of this depends on when the Tier1 method shows up -- in the above you can see it kicked in around iteration 7.

not clear yet why we can't hoist/cse this load. Need to look more closely what's going on.

       C5FB119C24A0000000   vmovsd   qword ptr [rsp+A0H], xmm3

x0 is modified in the inner loop. But it's unaliased over that scope. So presumably could be enregistered and spilled back to its stack home on loop exit. We don't have the technology to handle cases like this (scalar promotion or similar).

CSE does not consider these trees despite knowing that the two loads (at least) will return the same value.

N001 (  3,  4) [000110] ----G-------              \--*  LCL_VAR   double(AX) V03 loc2          $201
N001 (  3,  4) [000122] ----G-------              \--*  LCL_VAR   double(AX) V03 loc2          $201

presumably this runs into:

runtime/src/coreclr/jit/optcse.cpp

Lines 3576 to 3577 in 798d52b

    
           case GT_LCL_VAR: 
        
               return false; // Can't CSE a volatile LCL_VAR

which seems a bit short sighted? We know (as in this case) we have some invariant/expensive local var loads, why not try CSEing them if we have the room.

Will experiment.

we have some invariant/expensive local var loads, why not try CSEing them if we have the room.

Seems viable, though simply turning it on (even requiring cost >= IND_COST_EX) leads to a lot of code churn and a few asserts about incompatible types during CSE.

EG for benchmarks (via SPMI)

[14:25:42]
[14:25:42] Total bytes of base: 10418151 (overridden on cmd)
[14:25:42] Total bytes of diff: 10445323 (overridden on cmd)
[14:25:42] Total bytes of delta: 27172 (0.26 % of base)

[14:25:42] Top method regressions (bytes):
[14:25:42]          666 ( 6.63% of base) : 1247.dasm - DynamicClass:Regex2_TryMatchAtCurrentPosition(System.Text.RegularExpressions.RegexRunner,System.ReadOnlySpan`1[Char]):bool
[14:25:42]          546 (19.10% of base) : 9755.dasm - System.Xml.Serialization.XmlSerializationReaderILGen:GetArraySource(System.Xml.Serialization.TypeDesc,System.String,bool):System.String:this
[14:25:42]          486 (10.86% of base) : 9477.dasm - DynamicClass:Regex1_TryMatchAtCurrentPosition(System.Text.RegularExpressions.RegexRunner,System.ReadOnlySpan`1[Char]):bool
[14:25:42]          486 (10.86% of base) : 9480.dasm - DynamicClass:Regex1_TryMatchAtCurrentPosition(System.Text.RegularExpressions.RegexRunner,System.ReadOnlySpan`1[Char]):bool

[14:25:42] Top method improvements (bytes):
[14:25:42]         -601 (-13.04% of base) : 8376.dasm - Newtonsoft.Json.JsonWriter:WriteValue(Newtonsoft.Json.JsonWriter,int,System.Object)
[14:25:42]         -391 (-3.30% of base) : 2438.dasm - Utf8Json.Formatters.MicroBenchmarks_Serializers_LocationFormatter1:Serialize(byref,MicroBenchmarks.Serializers.Location,Utf8Json.IJsonFormatterResolver):this
[14:25:42]         -351 (-10.04% of base) : 26136.dasm - Microsoft.CodeAnalysis.CSharp.Syntax.InternalSyntax.LanguageParser:ParseNamespaceBody(byref,byref,byref,ushort):this
[14:25:42]         -306 (-21.32% of base) : 5590.dasm - System.Guid:TryParseExactN(System.ReadOnlySpan`1[Char],byref):bool
[14:25:42]         -302 (-19.48% of base) : 637.dasm - System.Guid:TryParseExactD(System.ReadOnlySpan`1[Char],byref):bool

[14:25:42] 3888 total methods with Code Size differences (1029 improved, 2859 regressed), 1627 unchanged.

I recall a while back I was touting something like this as an alternative to EH Write Through but never pushed on it.

Anyways, probably something too radical to land in a timely manner.

Latest update to BDN has made its way to perf repo and OSR runs. This should fix the ssl: true "regressions" where OSR consistently fared worse than main.

Seeing a new issue now in general OSR testing, presumably from the interaction of OSR and #66257.

Assert failure(PID 12384 [0x00003060], Thread: 10696 [0x29c8]): Assertion failed 'h2 != nullptr' in 'LUDecomp:DoLUIteration(System.Double[][],System.Double[],System.Double[][][],System.Double[][],int):long' during 'Clone loops' (IL size 144; hash 0x891a5971; Tier1-OSR)

The jit is trying to clone the outer loop here:

Cloning loop L00: [head: BB01, top: BB02, entry: BB04, bottom: BB09, child: L02].

Suspect the fix is simple? Here we have BB01->bbNext == BB04. So we decide not to create a new block for h2 here:

runtime/src/coreclr/jit/loopcloning.cpp

Lines 1903 to 1928 in ea4ebaa

    
           // Now we'll make "h2", after "h" to go to "e" -- unless the loop is a do-while, 
        
           // so that "h" already falls through to "e" (e == t). 
        
           // It might look like this code is unreachable, since "h" must be a BBJ_ALWAYS, but 
        
           // later we will change "h" to a BBJ_COND along with a set of loop conditions. 
        
           // TODO: it still might be unreachable, since cloning currently is restricted to "do-while" loop forms. 
        
           BasicBlock* h2 = nullptr; 
        
           if (h->bbNext != loop.lpEntry) 
        
           { 
        
               assert(h->bbJumpKind == BBJ_ALWAYS); 
        
               JITDUMP("Create branch to entry of optimized loop\n"); 
        
               BasicBlock* h2 = fgNewBBafter(BBJ_ALWAYS, h, /*extendRegion*/ true); 
        
               JITDUMP("Adding " FMT_BB " after " FMT_BB "\n", h2->bbNum, h->bbNum); 
        
               h2->bbWeight = h2->isRunRarely() ? BB_ZERO_WEIGHT : ambientWeight; 
        
               // This is in the scope of a surrounding loop, if one exists -- the parent of the loop we're cloning. 
        
               h2->bbNatLoopNum = ambientLoop; 
        
               h2->bbJumpDest = loop.lpEntry; 
        
               fgReplacePred(loop.lpEntry, h, h2); 
        
               JITDUMP("Replace " FMT_BB " -> " FMT_BB " with " FMT_BB " -> " FMT_BB "\n", h->bbNum, loop.lpEntry->bbNum, 
        
                       h2->bbNum, loop.lpEntry->bbNum); 
        
               optUpdateLoopHead(loopInd, h, h2); 
        
               // NOTE: 'h' is no longer the loop head; 'h2' is! 
        
           }

but BB01 does not transfer control to BB04.

Here we have BB01->bbNext == BB04

Doesn't BB01->bbNext == BB02? What is the actual lexical block order?

If you set COMPlus_JitDumpFgConstrained=1 what does it look like?

Here we have BB01->bbNext == BB04

Doesn't BB01->bbNext == BB02? What is the actual lexical block order?

Yeah, sorry, I was off base. As you noticed in #67067 we get into this code but weren't setting up the right h2.

Constrained view of the flowgraph:

There's one more libraries issue I want to investigate... on ubuntu x64

export COMPlus_TieredCompilation=1
export COMPlus_TC_OnStackReplacement=1
export COMPlus_TC_QuickJitForLoops=1

  Starting:    System.Text.Json.Tests (parallel test collections = on, max threads = 2)
    System.Text.Json.Tests.Utf8JsonReaderTests.ReadJsonStringsWithCommentsAndTrailingCommas(jsonString: "{\"Property1\": {\"Property1.1\": 42} // comment\n"...) [FAIL]
      System.Text.Json.JsonReaderException : The JSON object contains a trailing comma at the end which is not supported in this mode. Change the reader options. LineNumber: 1 | BytePositionInLine: 1
...

Issue for above #67152 -- fix in PR at #67274.

#83910 improved a couple of the microbenchmarks, notably

and might also fix some of the reported regressions -- taking a look.

@stephentoub

Seeing some benchmark cases where there are methods with stackalloc + loop that bypass tiering: #84264 (comment) and hence also bypass PGO.

In particular

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.TryGetProperty.cs

Lines 135 to 150 in f2a55e2

    
           private bool TryGetNamedPropertyValue( 
        
               int startIndex, 
        
               int endIndex, 
        
               ReadOnlySpan<byte> propertyName, 
        
               out JsonElement value) 
        
           { 
        
               ReadOnlySpan<byte> documentSpan = _utf8Json.Span; 
        
               Span<byte> utf8UnescapedStack = stackalloc byte[JsonConstants.StackallocByteThreshold]; 
        
               // Move to the row before the EndObject 
        
               int index = endIndex - DbRow.Size; 
        
               while (index > startIndex) 
        
               { 
        
                   DbRow row = _parsedData.Get(index); 
        
                   Debug.Assert(row.TokenType != JsonTokenType.PropertyName);

Not sure how common this is but something to keep an eye on. Supporting stackalloc in its full generality with OSR would be hard, because we potentially would need to track 3 addressable segments of the stack, but it's not impossible.

It might be easier to revise the BCL so this doesn't happen in places where we care about perf. The proposed mitigation would be to split the method into a caller that stackallocs and a callee that loops. These parts can be reunited (if deemed profitable) via normal inlining, or the callee marked with AggressiveInlining.

FYI @stephentoub -- possible pattern to avoid since it creates methods that can't benefit from Dynamic PGO.

Forked this off as #85548

Not sure how common this is but something to keep an eye on.

I think it is common because many developers (who cares about allocations and performance) are writing code like below nowadays.

const int StackAllocSize = 128;
Span<T> buffer = length < StackAllocSize ? stackalloc T[length] : new T[length];

Navigation Menu

Repository navigation

On Stack Replacement Next Steps #33658

Possible next steps now that #32969 is merged, in rough order of priority.

Issues and fixes after OSR was enabled

Performance Regressions

Other ideas: enhancements or optimizations

Design sketch for better supporting patchpoints within `try` regions

38 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	case GT_LCL_VAR:
	return false; // Can't CSE a volatile LCL_VAR

	// Now we'll make "h2", after "h" to go to "e" -- unless the loop is a do-while,
	// so that "h" already falls through to "e" (e == t).
	// It might look like this code is unreachable, since "h" must be a BBJ_ALWAYS, but
	// later we will change "h" to a BBJ_COND along with a set of loop conditions.
	// TODO: it still might be unreachable, since cloning currently is restricted to "do-while" loop forms.
	BasicBlock* h2 = nullptr;
	if (h->bbNext != loop.lpEntry)
	{
	assert(h->bbJumpKind == BBJ_ALWAYS);
	JITDUMP("Create branch to entry of optimized loop\n");
	BasicBlock* h2 = fgNewBBafter(BBJ_ALWAYS, h, /extendRegion/ true);
	JITDUMP("Adding " FMT_BB " after " FMT_BB "\n", h2->bbNum, h->bbNum);
	h2->bbWeight = h2->isRunRarely() ? BB_ZERO_WEIGHT : ambientWeight;

	// This is in the scope of a surrounding loop, if one exists -- the parent of the loop we're cloning.
	h2->bbNatLoopNum = ambientLoop;

	h2->bbJumpDest = loop.lpEntry;
	fgReplacePred(loop.lpEntry, h, h2);
	JITDUMP("Replace " FMT_BB " -> " FMT_BB " with " FMT_BB " -> " FMT_BB "\n", h->bbNum, loop.lpEntry->bbNum,
	h2->bbNum, loop.lpEntry->bbNum);

	optUpdateLoopHead(loopInd, h, h2);

	// NOTE: 'h' is no longer the loop head; 'h2' is!
	}

	private bool TryGetNamedPropertyValue(
	int startIndex,
	int endIndex,
	ReadOnlySpan<byte> propertyName,
	out JsonElement value)
	{
	ReadOnlySpan<byte> documentSpan = _utf8Json.Span;
	Span<byte> utf8UnescapedStack = stackalloc byte[JsonConstants.StackallocByteThreshold];

	// Move to the row before the EndObject
	int index = endIndex - DbRow.Size;

	while (index > startIndex)
	{
	DbRow row = _parsedData.Get(index);
	Debug.Assert(row.TokenType != JsonTokenType.PropertyName);

On Stack Replacement Next Steps #33658

Description

Possible next steps now that #32969 is merged, in rough order of priority.

Issues and fixes after OSR was enabled

Performance Regressions

Other ideas: enhancements or optimizations

Activity

AndyAyersMS commented on Mar 17, 2020

AndyAyersMS commented on Sep 16, 2021

Design sketch for better supporting patchpoints within try regions

AndyAyersMS commented on Sep 28, 2021

AndyAyersMS commented on Sep 28, 2021

AndyAyersMS commented on Nov 12, 2021

AndyAyersMS commented on Nov 15, 2021

AndyAyersMS commented on Nov 17, 2021

38 remaining items

AndyAyersMS commented on Mar 18, 2022

AndyAyersMS commented on Mar 18, 2022

AndyAyersMS commented on Mar 18, 2022

AndyAyersMS commented on Mar 23, 2022

AndyAyersMS commented on Mar 23, 2022

AndyAyersMS commented on Mar 23, 2022

AndyAyersMS commented on Mar 23, 2022

BruceForstall commented on Mar 23, 2022

AndyAyersMS commented on Mar 23, 2022

AndyAyersMS commented on Mar 25, 2022

AndyAyersMS commented on Mar 29, 2022

AndyAyersMS commented on Mar 30, 2023

AndyAyersMS commented on Apr 24, 2023

hez2010 commented on May 10, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

Design sketch for better supporting patchpoints within `try` regions