From e25f0d8366a89e2e9a7af2dc3ee2e827b36da0db Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 3 Apr 2023 13:48:22 -0700 Subject: [PATCH 1/4] Checkpoint endfinally prototype --- src/mono/mono/mini/interp/transform.c | 9 +++++++++ src/mono/wasm/runtime/jiterpreter-support.ts | 2 ++ .../runtime/jiterpreter-trace-generator.ts | 20 +++++++++++-------- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index e5b8719ce0f762..6e427f6c51ee03 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -745,6 +745,7 @@ handle_branch (TransformData *td, int long_op, int offset) if (offset < 0) target_bb->backwards_branch_target = TRUE; + if (offset < 0 && td->sp == td->stack && !td->inlined_method) { // Backwards branch inside unoptimized method where the IL stack is empty // This is candidate for a patchpoint @@ -8434,6 +8435,14 @@ generate_compacted_code (InterpMethod *rtm, TransformData *td) if (ins->opcode == MINT_TIER_PATCHPOINT_DATA) { int native_offset = (int)(ip - td->new_code); patchpoint_data_index = add_patchpoint_data (td, patchpoint_data_index, native_offset, -ins->data [0]); +#if HOST_BROWSER + } else if (rtm->contains_traces && ( + (ins->opcode == MINT_CALL_HANDLER_S) || (ins->opcode == MINT_CALL_HANDLER) + )) { + ip = emit_compacted_instruction (td, ip, ins); + if (backward_branch_offsets_count < BACKWARD_BRANCH_OFFSETS_SIZE) + backward_branch_offsets[backward_branch_offsets_count++] = ip - td->new_code; +#endif } else { ip = emit_compacted_instruction (td, ip, ins); } diff --git a/src/mono/wasm/runtime/jiterpreter-support.ts b/src/mono/wasm/runtime/jiterpreter-support.ts index fb481f3969b5e9..e842cb11b04cc2 100644 --- a/src/mono/wasm/runtime/jiterpreter-support.ts +++ b/src/mono/wasm/runtime/jiterpreter-support.ts @@ -157,6 +157,7 @@ export class WasmBuilder { options!: JiterpreterOptions; constantSlots: Array = []; backBranchOffsets: Array = []; + callHandlerReturnAddresses: Array = []; nextConstantSlot = 0; compressImportNames = false; @@ -202,6 +203,7 @@ export class WasmBuilder { for (let i = 0; i < this.constantSlots.length; i++) this.constantSlots[i] = 0; this.backBranchOffsets.length = 0; + this.callHandlerReturnAddresses.length = 0; this.allowNullCheckOptimization = this.options.eliminateNullChecks; } diff --git a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts index ed718df047e573..dc29aff6f0e275 100644 --- a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts +++ b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts @@ -345,12 +345,13 @@ export function generateWasmBody ( case MintOpcode.MINT_CALL_HANDLER_S: if (!emit_branch(builder, ip, frame, opcode)) ip = abort; - else + else { // Technically incorrect, but the instructions following this one may not be executed // since we might have skipped over them. // FIXME: Identify when we should actually set the conditionally executed flag, perhaps // by doing a simple static flow analysis based on the displacements. Update heuristic too! isConditionallyExecuted = true; + } break; case MintOpcode.MINT_CKNULL: { @@ -923,13 +924,15 @@ export function generateWasmBody ( isLowValueOpcode = true; break; - case MintOpcode.MINT_ENDFINALLY: - // This one might make sense to partially implement, but the jump target - // is computed at runtime which would make it hard to figure out where - // we need to put branch targets. Not worth just doing a conditional - // bailout since finally blocks always run - ip = abort; + case MintOpcode.MINT_ENDFINALLY: { + if (builder.callHandlerReturnAddresses.length) { + console.log(`endfinally @0x${(ip).toString(16)}. return addresses:`, builder.callHandlerReturnAddresses.map(ra => (ra).toString(16))); + ip = abort; + } else { + ip = abort; + } break; + } case MintOpcode.MINT_RETHROW: case MintOpcode.MINT_PROF_EXIT: @@ -2444,7 +2447,8 @@ function append_call_handler_store_ret_ip ( builder.appendU8(WasmOpcode.i32_store); builder.appendMemarg(clauseDataOffset, 0); // FIXME: 32-bit alignment? - // console.log(`call_handler clauseDataOffset=0x${clauseDataOffset.toString(16)} retIp=0x${retIp.toString(16)}`); + console.log(`call_handler @0x${(ip).toString(16)} retIp=0x${retIp.toString(16)}`); + builder.callHandlerReturnAddresses.push(retIp); } function emit_branch ( From 78c22cec0adde651e245a269b4d2b4579fb3f123 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Mon, 3 Apr 2023 16:06:15 -0700 Subject: [PATCH 2/4] Implement endfinally --- src/mono/wasm/runtime/jiterpreter-support.ts | 2 ++ .../runtime/jiterpreter-trace-generator.ts | 36 ++++++++++++++++--- src/mono/wasm/runtime/jiterpreter.ts | 3 ++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/mono/wasm/runtime/jiterpreter-support.ts b/src/mono/wasm/runtime/jiterpreter-support.ts index e842cb11b04cc2..887a32c2cc6344 100644 --- a/src/mono/wasm/runtime/jiterpreter-support.ts +++ b/src/mono/wasm/runtime/jiterpreter-support.ts @@ -50,6 +50,7 @@ export const enum BailoutReason { CallDelegate, Debugging, Icall, + UnexpectedRetIp, } export const BailoutReasonNames = [ @@ -78,6 +79,7 @@ export const BailoutReasonNames = [ "CallDelegate", "Debugging", "Icall", + "UnexpectedRetIp", ]; type FunctionType = [ diff --git a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts index dc29aff6f0e275..1a026b889a020b 100644 --- a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts +++ b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts @@ -27,6 +27,7 @@ import { traceEip, nullCheckValidation, abortAtJittedLoopBodies, traceNullCheckOptimizations, nullCheckCaching, traceBackBranches, + enableEndFinally, mostRecentOptions, @@ -925,9 +926,36 @@ export function generateWasmBody ( break; case MintOpcode.MINT_ENDFINALLY: { - if (builder.callHandlerReturnAddresses.length) { - console.log(`endfinally @0x${(ip).toString(16)}. return addresses:`, builder.callHandlerReturnAddresses.map(ra => (ra).toString(16))); - ip = abort; + if ( + enableEndFinally && + (builder.callHandlerReturnAddresses.length > 0) && + (builder.callHandlerReturnAddresses.length <= 3) + ) { + // console.log(`endfinally @0x${(ip).toString(16)}. return addresses:`, builder.callHandlerReturnAddresses.map(ra => (ra).toString(16))); + // FIXME: Clean this codegen up + // Load ret_ip + const clauseIndex = getArgU16(ip, 1), + clauseDataOffset = get_imethod_clause_data_offset(frame, clauseIndex); + builder.local("pLocals"); + builder.appendU8(WasmOpcode.i32_load); + builder.appendMemarg(clauseDataOffset, 0); + // Stash it in a variable because we're going to need to use it multiple times + builder.local("math_lhs32", WasmOpcode.set_local); + // Do a bunch of trivial comparisons to see if ret_ip is one of our expected return addresses, + // and if it is, generate a branch back to the dispatcher at the top + for (let r = 0; r < builder.callHandlerReturnAddresses.length; r++) { + const ra = builder.callHandlerReturnAddresses[r]; + builder.local("math_lhs32"); + builder.ptr_const(ra); + builder.appendU8(WasmOpcode.i32_eq); + builder.block(WasmValtype.void, WasmOpcode.if_); + builder.cfg.branch(ra, ra < ip, true); + builder.endBlock(); + } + // If none of the comparisons succeeded we won't have branched anywhere, so bail out + // This shouldn't happen during non-exception-handling execution unless the trace doesn't + // contain the CALL_HANDLER that led here + append_bailout(builder, ip, BailoutReason.UnexpectedRetIp); } else { ip = abort; } @@ -2447,7 +2475,7 @@ function append_call_handler_store_ret_ip ( builder.appendU8(WasmOpcode.i32_store); builder.appendMemarg(clauseDataOffset, 0); // FIXME: 32-bit alignment? - console.log(`call_handler @0x${(ip).toString(16)} retIp=0x${retIp.toString(16)}`); + // console.log(`call_handler @0x${(ip).toString(16)} retIp=0x${retIp.toString(16)}`); builder.callHandlerReturnAddresses.push(retIp); } diff --git a/src/mono/wasm/runtime/jiterpreter.ts b/src/mono/wasm/runtime/jiterpreter.ts index 5a0e8417bcddb6..d7e8cdc2914cdd 100644 --- a/src/mono/wasm/runtime/jiterpreter.ts +++ b/src/mono/wasm/runtime/jiterpreter.ts @@ -63,6 +63,9 @@ export const // Unproductive if we have backward branches enabled because it can stop us from jitting // nested loops abortAtJittedLoopBodies = true, + // Enable generating conditional backward branches for ENDFINALLY opcodes if we saw some CALL_HANDLER + // opcodes previously + enableEndFinally = true, // Emit a wasm nop between each managed interpreter opcode emitPadding = false, // Generate compressed names for imports so that modules have more space for code From 44eb191375a1d2ea4c76f0c91becd4f93c874696 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 4 Apr 2023 15:08:27 -0700 Subject: [PATCH 3/4] Address PR feedback Logging cleanup Don't generate dispatch entries for back branches we haven't seen targeted --- src/mono/mono/mini/interp/transform.c | 4 +++- src/mono/wasm/runtime/jiterpreter-support.ts | 17 ++++++++++++---- .../runtime/jiterpreter-trace-generator.ts | 20 ++++++++++++------- src/mono/wasm/runtime/jiterpreter.ts | 16 ++++++++------- 4 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/mono/mono/mini/interp/transform.c b/src/mono/mono/mini/interp/transform.c index 6e427f6c51ee03..0c0ea50d698749 100644 --- a/src/mono/mono/mini/interp/transform.c +++ b/src/mono/mono/mini/interp/transform.c @@ -745,7 +745,6 @@ handle_branch (TransformData *td, int long_op, int offset) if (offset < 0) target_bb->backwards_branch_target = TRUE; - if (offset < 0 && td->sp == td->stack && !td->inlined_method) { // Backwards branch inside unoptimized method where the IL stack is empty // This is candidate for a patchpoint @@ -8439,6 +8438,9 @@ generate_compacted_code (InterpMethod *rtm, TransformData *td) } else if (rtm->contains_traces && ( (ins->opcode == MINT_CALL_HANDLER_S) || (ins->opcode == MINT_CALL_HANDLER) )) { + // While this formally isn't a backward branch target, we want to record + // the offset of its following instruction so that the jiterpreter knows + // to generate the necessary dispatch code to enable branching back to it. ip = emit_compacted_instruction (td, ip, ins); if (backward_branch_offsets_count < BACKWARD_BRANCH_OFFSETS_SIZE) backward_branch_offsets[backward_branch_offsets_count++] = ip - td->new_code; diff --git a/src/mono/wasm/runtime/jiterpreter-support.ts b/src/mono/wasm/runtime/jiterpreter-support.ts index 887a32c2cc6344..e84a0fe257cd84 100644 --- a/src/mono/wasm/runtime/jiterpreter-support.ts +++ b/src/mono/wasm/runtime/jiterpreter-support.ts @@ -1013,6 +1013,7 @@ class Cfg { blockStack: Array = []; backDispatchOffsets: Array = []; dispatchTable = new Map(); + observedBranchTargets = new Set(); trace = 0; constructor (builder: WasmBuilder) { @@ -1029,6 +1030,7 @@ class Cfg { this.lastSegmentEnd = 0; this.overheadBytes = 10; // epilogue this.dispatchTable.clear(); + this.observedBranchTargets.clear(); this.trace = trace; this.backDispatchOffsets.length = 0; } @@ -1075,6 +1077,7 @@ class Cfg { } branch (target: MintOpcodePtr, isBackward: boolean, isConditional: boolean) { + this.observedBranchTargets.add(target); this.appendBlob(); this.segments.push({ type: "branch", @@ -1144,13 +1147,19 @@ class Cfg { this.backDispatchOffsets.length = 0; // First scan the back branch target table and union it with the block stack // This filters down to back branch targets that are reachable inside this trace + // Further filter it down by only including targets we have observed a branch to + // this helps for cases where the branch opcodes targeting the location were not + // compiled due to an abort or some other reason for (let i = 0; i < this.backBranchTargets.length; i++) { const offset = (this.backBranchTargets[i] * 2) + this.startOfBody; const breakDepth = this.blockStack.indexOf(offset); - if (breakDepth >= 0) { - this.dispatchTable.set(offset, this.backDispatchOffsets.length + 1); - this.backDispatchOffsets.push(offset); - } + if (breakDepth < 0) + continue; + if (!this.observedBranchTargets.has(offset)) + continue; + + this.dispatchTable.set(offset, this.backDispatchOffsets.length + 1); + this.backDispatchOffsets.push(offset); } if (this.backDispatchOffsets.length === 0) { diff --git a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts index 1a026b889a020b..758d180f9d4fcc 100644 --- a/src/mono/wasm/runtime/jiterpreter-trace-generator.ts +++ b/src/mono/wasm/runtime/jiterpreter-trace-generator.ts @@ -27,7 +27,7 @@ import { traceEip, nullCheckValidation, abortAtJittedLoopBodies, traceNullCheckOptimizations, nullCheckCaching, traceBackBranches, - enableEndFinally, + maxCallHandlerReturnAddresses, mostRecentOptions, @@ -927,9 +927,8 @@ export function generateWasmBody ( case MintOpcode.MINT_ENDFINALLY: { if ( - enableEndFinally && (builder.callHandlerReturnAddresses.length > 0) && - (builder.callHandlerReturnAddresses.length <= 3) + (builder.callHandlerReturnAddresses.length <= maxCallHandlerReturnAddresses) ) { // console.log(`endfinally @0x${(ip).toString(16)}. return addresses:`, builder.callHandlerReturnAddresses.map(ra => (ra).toString(16))); // FIXME: Clean this codegen up @@ -2528,10 +2527,14 @@ function emit_branch ( counters.backBranchesEmitted++; return true; } else { - if ((traceBackBranches > 0) || (builder.cfg.trace > 0)) - console.log(`back branch target 0x${destination.toString(16)} not found in list ` + + if (destination < builder.cfg.entryIp) { + if ((traceBackBranches > 1) || (builder.cfg.trace > 1)) + console.log(`${info[0]} target 0x${destination.toString(16)} before start of trace`); + } else if ((traceBackBranches > 0) || (builder.cfg.trace > 0)) + console.log(`0x${(ip).toString(16)} ${info[0]} target 0x${destination.toString(16)} not found in list ` + builder.backBranchOffsets.map(bbo => "0x" + (bbo).toString(16)).join(", ") ); + cwraps.mono_jiterp_boost_back_branch_target(destination); // FIXME: Should there be a safepoint here? append_bailout(builder, destination, BailoutReason.BackwardBranch); @@ -2618,8 +2621,11 @@ function emit_branch ( builder.cfg.branch(destination, true, true); counters.backBranchesEmitted++; } else { - if ((traceBackBranches > 0) || (builder.cfg.trace > 0)) - console.log(`back branch target 0x${destination.toString(16)} not found in list ` + + if (destination < builder.cfg.entryIp) { + if ((traceBackBranches > 1) || (builder.cfg.trace > 1)) + console.log(`${info[0]} target 0x${destination.toString(16)} before start of trace`); + } else if ((traceBackBranches > 0) || (builder.cfg.trace > 0)) + console.log(`0x${(ip).toString(16)} ${info[0]} target 0x${destination.toString(16)} not found in list ` + builder.backBranchOffsets.map(bbo => "0x" + (bbo).toString(16)).join(", ") ); // We didn't find a loop to branch to, so bail out diff --git a/src/mono/wasm/runtime/jiterpreter.ts b/src/mono/wasm/runtime/jiterpreter.ts index d7e8cdc2914cdd..cd1ced5cf31689 100644 --- a/src/mono/wasm/runtime/jiterpreter.ts +++ b/src/mono/wasm/runtime/jiterpreter.ts @@ -64,8 +64,12 @@ export const // nested loops abortAtJittedLoopBodies = true, // Enable generating conditional backward branches for ENDFINALLY opcodes if we saw some CALL_HANDLER - // opcodes previously - enableEndFinally = true, + // opcodes previously, up to this many potential return addresses. If a trace contains more potential + // return addresses than this we will not emit code for the ENDFINALLY opcode + maxCallHandlerReturnAddresses = 3, + // Controls how many individual items (traces, bailouts, etc) are shown in the breakdown + // at the end of a run when stats are enabled. The N highest ranking items will be shown. + summaryStatCount = 30, // Emit a wasm nop between each managed interpreter opcode emitPadding = false, // Generate compressed names for imports so that modules have more space for code @@ -987,7 +991,7 @@ export function jiterpreter_dump_stats (b?: boolean, concise?: boolean) { console.log(`// traces bailed out ${bailoutCount} time(s) due to ${BailoutReasonNames[i]}`); } - for (let i = 0, c = 0; i < traces.length && c < 30; i++) { + for (let i = 0, c = 0; i < traces.length && c < summaryStatCount; i++) { const trace = traces[i]; if (!trace.bailoutCount) continue; @@ -1019,7 +1023,7 @@ export function jiterpreter_dump_stats (b?: boolean, concise?: boolean) { console.log("// hottest call targets:"); const targetPointers = Object.keys(callTargetCounts); targetPointers.sort((l, r) => callTargetCounts[Number(r)] - callTargetCounts[Number(l)]); - for (let i = 0, c = Math.min(20, targetPointers.length); i < c; i++) { + for (let i = 0, c = Math.min(summaryStatCount, targetPointers.length); i < c; i++) { const targetMethod = Number(targetPointers[i]) | 0; const pMethodName = cwraps.mono_wasm_method_get_full_name(targetMethod); const targetMethodName = Module.UTF8ToString(pMethodName); @@ -1031,7 +1035,7 @@ export function jiterpreter_dump_stats (b?: boolean, concise?: boolean) { traces.sort((l, r) => r.hitCount - l.hitCount); console.log("// hottest failed traces:"); - for (let i = 0, c = 0; i < traces.length && c < 20; i++) { + for (let i = 0, c = 0; i < traces.length && c < summaryStatCount; i++) { // this means the trace has a low hit count and we don't know its identity. no value in // logging it. if (!traces[i].name) @@ -1067,7 +1071,6 @@ export function jiterpreter_dump_stats (b?: boolean, concise?: boolean) { case "newobj_slow": case "switch": case "rethrow": - case "endfinally": case "end-of-body": case "ret": continue; @@ -1075,7 +1078,6 @@ export function jiterpreter_dump_stats (b?: boolean, concise?: boolean) { // not worth implementing / too difficult case "intrins_marvin_block": case "intrins_ascii_chars_to_uppercase": - case "newarr": continue; } } From 3d36048f75b76b1ac20e9fcd9df67734fd366e93 Mon Sep 17 00:00:00 2001 From: Katelyn Gadd Date: Tue, 4 Apr 2023 16:35:01 -0700 Subject: [PATCH 4/4] Update heuristic for endfinally support Add browser-bench measurement for try-finally --- src/mono/mono/mini/interp/jiterpreter.c | 5 ++++- .../sample/wasm/browser-bench/Exceptions.cs | 22 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/mono/mono/mini/interp/jiterpreter.c b/src/mono/mono/mini/interp/jiterpreter.c index 9fa216f684810e..bcd55db17255a2 100644 --- a/src/mono/mono/mini/interp/jiterpreter.c +++ b/src/mono/mono/mini/interp/jiterpreter.c @@ -731,6 +731,10 @@ jiterp_should_abort_trace (InterpInst *ins, gboolean *inside_branch_block) return TRACE_CONTINUE; + case MINT_ENDFINALLY: + // May produce either a backwards branch or a bailout + return TRACE_CONDITIONAL_ABORT; + case MINT_ICALL_V_P: case MINT_ICALL_V_V: case MINT_ICALL_P_P: @@ -748,7 +752,6 @@ jiterp_should_abort_trace (InterpInst *ins, gboolean *inside_branch_block) case MINT_LEAVE_S_CHECK: return TRACE_ABORT; - case MINT_ENDFINALLY: case MINT_RETHROW: case MINT_PROF_EXIT: case MINT_PROF_EXIT_VOID: diff --git a/src/mono/sample/wasm/browser-bench/Exceptions.cs b/src/mono/sample/wasm/browser-bench/Exceptions.cs index 2619fc4cfb70cc..ffe96e89e513f1 100644 --- a/src/mono/sample/wasm/browser-bench/Exceptions.cs +++ b/src/mono/sample/wasm/browser-bench/Exceptions.cs @@ -22,6 +22,7 @@ public ExceptionsTask() new TryCatchFilterInline(), new TryCatchFilterThrow(), new TryCatchFilterThrowApplies(), + new TryFinally(), }; } @@ -208,5 +209,26 @@ void DoThrow() throw new System.Exception("Reached DoThrow and threw"); } } + + class TryFinally : ExcMeasurement + { + public override string Name => "TryFinally"; + int j = 1; + + public override void RunStep() + { + int i = 0; + try + { + i += j; + } + finally + { + i += j; + } + if (i != 2) + throw new System.Exception("Internal error"); + } + } } }