[interp] Remove saving of clause_args from interp state (#37048)

clause_args can be set only from EH. This means that clause_args can be set only in the first executed frame, so there is no need to save it in the interp state. Also make sure we never check for it in the main_loop block, for normal calls and returns. Added exec_frame to the clause_args so we can detect whether a frame is the first frame invoked from EH. We can no longer use clause_args for this purpose because we no longer save it in the interp state.

Makes calls 6% faster

Co-authored-by: BrzVlad <BrzVlad@users.noreply.github.com>

diff --git a/src/mono/mono/mini/interp/interp-internals.h b/src/mono/mono/mini/interp/interp-internals.h
index b88e969..75352fd 100644 (file)
--- a/src/mono/mono/mini/interp/interp-internals.h
+++ b/src/mono/mono/mini/interp/interp-internals.h
@@ -205,7 +205,6 @@ typedef struct {
        unsigned char *vt_sp;
        const unsigned short  *ip;
        GSList *finally_ips;
-       FrameClauseArgs *clause_args;
 } InterpState;
 
 struct InterpFrame {

diff --git a/src/mono/mono/mini/interp/interp.c b/src/mono/mono/mini/interp/interp.c
index ed8559c..15656c8 100644 (file)
--- a/src/mono/mono/mini/interp/interp.c
+++ b/src/mono/mono/mini/interp/interp.c
@@ -93,6 +93,9 @@ struct FrameClauseArgs {
        int exit_clause;
        /* Exception that we are filtering */
        MonoException *filter_exception;
+       /* Frame that is executing this clause */
+       InterpFrame *exec_frame;
+       /* If set, exec_frame is a duplicate separate frame of this frame */
        InterpFrame *base_frame;
 };
 
@@ -280,7 +283,7 @@ static gboolean ss_enabled;
 static gboolean interp_init_done = FALSE;
 
 static void
-interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs *clause_args, MonoError *error);
+interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs *clause_args);
 
 static InterpMethod* lookup_method_pointer (gpointer addr);
 
@@ -1896,7 +1899,7 @@ interp_runtime_invoke (MonoMethod *method, void *obj, void **params, MonoObject
 
        InterpFrame frame = {NULL, imethod, args, &result};
 
-       interp_exec_method (&frame, context, NULL, error);
+       interp_exec_method (&frame, context, NULL);
 
        if (context->has_resume_state) {
                // This can happen on wasm !?
@@ -2001,8 +2004,7 @@ interp_entry (InterpEntryData *data)
                break;
        }
 
-       ERROR_DECL (error);
-       interp_exec_method (&frame, context, NULL, error);
+       interp_exec_method (&frame, context, NULL);
 
        g_assert (!context->has_resume_state);
 
@@ -2740,8 +2742,7 @@ interp_entry_from_trampoline (gpointer ccontext_untyped, gpointer rmethod_untype
        /* Copy the args saved in the trampoline to the frame stack */
        mono_arch_get_native_call_context_args (ccontext, &frame, sig);
 
-       ERROR_DECL (error);
-       interp_exec_method (&frame, context, NULL, error);
+       interp_exec_method (&frame, context, NULL);
 
        g_assert (!context->has_resume_state);
 
@@ -3331,7 +3332,6 @@ method_entry (ThreadContext *context, InterpFrame *frame,
        frame->state.sp = sp; \
        frame->state.vt_sp = vt_sp; \
        frame->state.finally_ips = finally_ips; \
-       frame->state.clause_args = clause_args; \
        } while (0)
 
 /* Load and clear state from FRAME */
@@ -3340,14 +3340,13 @@ method_entry (ThreadContext *context, InterpFrame *frame,
        sp = frame->state.sp; \
        vt_sp = frame->state.vt_sp; \
        finally_ips = frame->state.finally_ips; \
-       clause_args = frame->state.clause_args; \
        locals = (unsigned char *)frame->stack + frame->imethod->stack_size + frame->imethod->vt_stack_size; \
        frame->state.ip = NULL; \
        } while (0)
 
 /* Initialize interpreter state for executing FRAME */
 #define INIT_INTERP_STATE(frame, _clause_args) do {     \
-       ip = _clause_args ? (_clause_args)->start_with_ip : (frame)->imethod->code; \
+       ip = _clause_args ? ((FrameClauseArgs *)_clause_args)->start_with_ip : (frame)->imethod->code; \
        sp = (frame)->stack; \
        vt_sp = (unsigned char *) sp + (frame)->imethod->stack_size; \
        locals = (unsigned char *) vt_sp + (frame)->imethod->vt_stack_size; \
@@ -3361,10 +3360,11 @@ method_entry (ThreadContext *context, InterpFrame *frame,
  * FRAME is only valid until the next call to alloc_frame ().
  */
 static MONO_NEVER_INLINE void
-interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs *clause_args, MonoError *error)
+interp_exec_method (InterpFrame *frame, ThreadContext *context, FrameClauseArgs *clause_args)
 {
        InterpMethod *cmethod;
        MonoException *ex;
+       ERROR_DECL(error);
 
        /* Interpreter main loop state (InterpState) */
        const guint16 *ip = NULL;
@@ -3925,8 +3925,7 @@ call:
                                EXCEPTION_CHECKPOINT;
                        }
 
-                       clause_args = NULL;
-                       INIT_INTERP_STATE (frame, clause_args);
+                       INIT_INTERP_STATE (frame, NULL);
 
                        MINT_IN_BREAK;
                }
@@ -6338,8 +6337,12 @@ call_newobj:
                        // After mono_threads_end_abort_protected_block to conserve stack.
                        const int clause_index = *ip;
 
-                       if (clause_args && clause_index == clause_args->exit_clause)
-                               goto exit_frame;
+                       // clause_args stores the clause args only for the first frame that
+                       // we started executing in interp_exec_method. If we are exiting the
+                       // current frame at this finally clause, we need to make sure that
+                       // this is the first frame invoked with interp_exec_method.
+                       if (clause_args && clause_args->exec_frame == frame && clause_index == clause_args->exit_clause)
+                               goto exit_clause;
 
 #if DEBUG_INTERP // This assert causes Linux/amd64/clang to use more stack.
                        g_assert (sp >= frame->stack);
@@ -6925,7 +6928,7 @@ call_newobj:
                MINT_IN_CASE(MINT_ENDFILTER)
                        /* top of stack is result of filter */
                        frame->retval->data.i = sp [-1].data.i;
-                       goto exit_frame;
+                       goto exit_clause;
                MINT_IN_CASE(MINT_INITOBJ)
                        --sp;
                        memset (sp->data.vt, 0, READ32(ip + 1));
@@ -7178,37 +7181,42 @@ resume:
        g_assert (context->has_resume_state);
        g_assert (frame->imethod);
 
-       if (frame == context->handler_frame && (!clause_args || context->handler_ip < clause_args->end_at_ip)) {
-               /* Set the current execution state to the resume state in context */
-
-               ip = context->handler_ip;
-               /* spec says stack should be empty at endfinally so it should be at the start too */
-               sp = frame->stack;
-               vt_sp = (guchar*)sp + frame->imethod->stack_size;
-               g_assert (context->exc_gchandle);
-               sp->data.p = mono_gchandle_get_target_internal (context->exc_gchandle);
-               ++sp;
+       if (frame == context->handler_frame) {
+               /*
+                * When running finally blocks, we can have the same frame twice on the stack. If we have
+                * clause_args information, we need to check whether resuming should happen inside this
+                * finally block, or in some other part of the method, in which case we need to exit.
+                */
+               if (clause_args && frame == clause_args->exec_frame && context->handler_ip >= clause_args->end_at_ip) {
+                       goto exit_clause;
+               } else {
+                       /* Set the current execution state to the resume state in context */
+                       ip = context->handler_ip;
+                       /* spec says stack should be empty at endfinally so it should be at the start too */
+                       sp = frame->stack;
+                       vt_sp = (guchar*)sp + frame->imethod->stack_size;
+                       g_assert (context->exc_gchandle);
+                       sp->data.p = mono_gchandle_get_target_internal (context->exc_gchandle);
+                       ++sp;
 
-               finally_ips = clear_resume_state (context, finally_ips);
-               // goto main_loop instead of MINT_IN_DISPATCH helps the compiler and therefore conserves stack.
-               // This is a slow/rare path and conserving stack is preferred over its performance otherwise.
-               goto main_loop;
+                       finally_ips = clear_resume_state (context, finally_ips);
+                       // goto main_loop instead of MINT_IN_DISPATCH helps the compiler and therefore conserves stack.
+                       // This is a slow/rare path and conserving stack is preferred over its performance otherwise.
+                       goto main_loop;
+               }
+       } else if (clause_args && frame == clause_args->exec_frame) {
+               /*
+                * This frame doesn't handle the resume state and it is the first frame invoked from EH.
+                * We can't just return to parent. We must first exit the EH mechanism and start resuming
+                * again from the original frame.
+                */
+               goto exit_clause;
        }
        // fall through
 exit_frame:
-       error_init_reuse (error);
-
        g_assert_checked (frame->imethod);
 
-       if (clause_args && clause_args->base_frame) {
-               // We finished executing a filter. The execution stack of the base frame
-               // should remain unmodified, but we need to update the local space.
-               char *locals_base = (char*)clause_args->base_frame->stack + frame->imethod->stack_size + frame->imethod->vt_stack_size;
-
-               memcpy (locals_base, locals, frame->imethod->locals_size);
-       }
-
-       if (!clause_args && frame->parent && frame->parent->state.ip) {
+       if (frame->parent && frame->parent->state.ip) {
                /* Return to the main loop after a non-recursive interpreter call */
                //printf ("R: %s -> %s %p\n", mono_method_get_full_name (frame->imethod->method), mono_method_get_full_name (frame->parent->imethod->method), frame->parent->state.ip);
                InterpFrame* const child_frame = frame;
@@ -7222,9 +7230,19 @@ exit_frame:
 
                goto main_loop;
        }
+exit_clause:
+       if (clause_args) {
+               if (clause_args->base_frame) {
+                       // We finished executing a filter. The execution stack of the base frame
+                       // should remain unmodified, but we need to update the local space.
+                       char *locals_base = (char*)clause_args->base_frame->stack + frame->imethod->stack_size + frame->imethod->vt_stack_size;
 
-       if (!clause_args)
+                       memcpy (locals_base, locals, frame->imethod->locals_size);
+                       pop_frame (context, frame);
+               }
+       } else {
                pop_frame (context, frame);
+       }
 
        DEBUG_LEAVE ();
 }
@@ -7316,6 +7334,7 @@ interp_run_finally (StackFrameInfo *frame, int clause_index, gpointer handler_ip
        clause_args.start_with_ip = (const guint16*)handler_ip;
        clause_args.end_at_ip = (const guint16*)handler_ip_end;
        clause_args.exit_clause = clause_index;
+       clause_args.exec_frame = iframe;
 
        state_ip = iframe->state.ip;
        iframe->state.ip = NULL;
@@ -7323,12 +7342,10 @@ interp_run_finally (StackFrameInfo *frame, int clause_index, gpointer handler_ip
        InterpFrame* const next_free = iframe->next_free;
        iframe->next_free = NULL;
 
-       ERROR_DECL (error);
-       interp_exec_method (iframe, context, &clause_args, error);
+       interp_exec_method (iframe, context, &clause_args);
 
        iframe->next_free = next_free;
        iframe->state.ip = state_ip;
-       iframe->state.clause_args = NULL;
        if (context->has_resume_state) {
                return TRUE;
        } else {
@@ -7363,9 +7380,9 @@ interp_run_filter (StackFrameInfo *frame, MonoException *ex, int clause_index, g
        clause_args.end_at_ip = (const guint16*)handler_ip_end;
        clause_args.filter_exception = ex;
        clause_args.base_frame = iframe;
+       clause_args.exec_frame = &child_frame;
 
-       ERROR_DECL (error);
-       interp_exec_method (&child_frame, context, &clause_args, error);
+       interp_exec_method (&child_frame, context, &clause_args);
 
        /* ENDFILTER stores the result into child_frame->retval */
        return retval.data.i ? TRUE : FALSE;