Re: Storing Bytecode Offset

[Tom Tromey]

>>>>> "Zach" == Zach Shaftel <zshaftel@gmail.com> writes:

Zach> Rocky has pushed the code to Savannah. In the
Zach> feature/zach-soc-funcall-from-bytecode branch
[...]
Zach> But the code on that branch is just
Zach> a rough proof-of-concept and isn't 100% accurate. Doing it right would
Zach> require a lot of refactoring that could ultimately lead to a dead end.

Could you say more specifically what is wrong on that branch?  What kind
of refactoring is needed?

I think other interpreters out there take this "self-call" approach, and
it seems like a decent idea to try in Emacs.

Zach> In the feature/zach-soc-bytecode-in-traceback branch, the offset is
Zach> stored in the thread_state struct. Prior to the most recent commit, the
Zach> offset was then stashed in the backtrace specbinding frame from
Zach> record_in_backtrace, so each bytecode call in the backtrace buffer was
Zach> annotated with the offset when the error occurred. This is the ultimate
Zach> goal, but the current implementation is flawed and a significant source
Zach> of slowdown.

I took a brief look at it.  One thing I noticed is that the assignment
is done in NEXT:

+#define NEXT UPDATE_OFFSET; goto *(targets[op = FETCH])

However, it seems to me that there cases where this is not needed -- any
bytecode that cannot possibly cause an exception doesn't need to record
the offset.  For example:

        CASE (Bvarref6):
          op = FETCH;
        varref:
          {
            Lisp_Object v1 = vectorp[op], v2;
            if (!SYMBOLP (v1)
                || XSYMBOL (v1)->u.s.redirect != SYMBOL_PLAINVAL
                || (v2 = SYMBOL_VAL (XSYMBOL (v1)), EQ (v2, Qunbound)))
              v2 = Fsymbol_value (v1);
            PUSH (v2);
            NEXT;
          }

Here, I think the updating only has to be done before the call to
Fsymbol_value.

Or:

        CASE (Bgotoifnil):
          {
            Lisp_Object v1 = POP;
            op = FETCH2;
            if (NILP (v1))
              goto op_branch;
            NEXT;
          }

This opcode doesn't need to update the offset at all.

This matters because the change is introducing an extra store into
performance-sensitive code.

I had a couple of other ideas for how to micro-optimize this store.
I don't know if they will work, but you might try them.

The updating looks like this:

+#define UPDATE_OFFSET (backtrace_byte_offset = pc - bytestr_data)

If you also stored bytestr_data in the structure, then instead of doing
the subtraction in the hot loop, you could do the subtraction when
computing the location -- which happens more rarely, and which is not
performance-sensitive.


Also, as you said, backtrace_byte_offset actually references a field in
the thread state:

+  /* The offset of the current op of the byte-code function being
+     executed. */
+  int m_backtrace_byte_offset;
+#define backtrace_byte_offset (current_thread->m_backtrace_byte_offset)

You might try changing this to be a pointer, and have it point to a
local variable that is updated by UPDATE_OFFSET.  This might be a bit
faster due to cache effects.

Note that you do *not* want to have it point to the actual "pc" variable
-- if you take the address of "pc", the compiler will probably not put
it in a register.  I mean, you might try it, but I would expect a
performance loss in this case.

Tom