Minimize the amount of calls into `event.bp`

[thewhitegoatcb]

Resolves https://github.com/actboy168/lua-debug/issues/259 Needs more testing for non jit lua as I'm not an expert. The performance boost is over 100x for awaiting breakpoints. Tweaks are done by @fesily suggestions

[fesily]

First of all, one thing is clear, this pull should be a generic one and not a luajit specific one.

Secondly, the code now has to call event.bp because lua-debug is not sure if the proto and source information of the current lua function has been read? @actboy168

[thewhitegoatcb]

First of all, one thing is clear, this pull should be a generic one and not a luajit specific one.

Secondly, the code now has to call event.bp because lua-debug is not sure if the proto and source information of the current lua function has been read? @actboy168

wouldn't that be done in newproto that's called by break_new?

[fesily]

First of all, one thing is clear, this pull should be a generic one and not a luajit specific one. Secondly, the code now has to call event.bp because lua-debug is not sure if the proto and source information of the current lua function has been read? @actboy168

wouldn't that be done in newproto that's called by break_new?

So this question needs to be answered by actboy168. As I understand it bp events are only valid in the bpmap::status::Break state.

[thewhitegoatcb]

In my understanding, The issue arises due to an assumption that case LUA_HOOKLINE: is only executed when we found a breakpoint on the previous hook call. That would be wrong in luajit's case, because we are doing this

#ifdef LUAJIT_VERSION
            last_hook_call_in_c = !lua_isluafunc(hL, ar);
            if (last_hook_call_in_c) {
                thread_mask |= LUA_MASKLINE;
                updatehookmask(hL);

                if (update_mask)
                    update_hook(hL);
            }
#endif

in LUA_HOOKCALL, LUA_HOOKTAILCALL, LUA_HOOKTAILRET, hence we getting lots of event.bp on C func exits. I don't think it's the intended use of it, as it wouldn't happen in standard Lua (I didn't test, but the code seems to indicate that).

[fesily]

First of all, one thing is clear, this pull should be a generic one and not a luajit specific one. Secondly, the code now has to call event.bp because lua-debug is not sure if the proto and source information of the current lua function has been read? @actboy168

wouldn't that be done in newproto that's called by break_new?

So this question needs to be answered by actboy168. As I understand it bp events are only valid in the bpmap::status::Break state.

You found the core problem, I do not see. It should be here to deal with it. I was calling the c function because when I stepped in, the hit position would not be the second line of the function

[fesily]

@thewhitegoatcb

void full_hook(lua_State* hL, lua_Debug* ar) {
        const auto is_step_mode = [this]() { return (step_mask & LUA_MASKLINE) && (!stepL || stepL == hL); };
        switch (ar->event) {
        case LUA_HOOKLINE:
#ifdef LUAJIT_VERSION
            if (last_hook_call_in_c) {
#    if defined(LUA_HOOKTHREAD)
                thread_mask &= (~LUA_MASKTHREAD);
#    endif
                updatehookmask(hL);
                last_hook_call_in_c = false;
                if (break_mask)
                    break_hook_return_luajit(hL, ar);
            }
            if (stepL == hL) {
                if (step_mask & LUA_MASKRET) {
                    step_hook_line(hL, ar);
                }
            }
            if (!is_step_mode() && !(break_mask & LUA_MASKLINE))
                return;
#endif
            break;

try the code. break_hook_return_luajit = break_hook_call

[fesily]

@thewhitegoatcb the code maybe better

diff --git a/src/luadebug/rdebug_hookmgr.cpp b/src/luadebug/rdebug_hookmgr.cpp
index 5331e3c0..b86f6abf 100644
--- a/src/luadebug/rdebug_hookmgr.cpp
+++ b/src/luadebug/rdebug_hookmgr.cpp
@@ -392,11 +392,22 @@ struct hookmgr {
 #    endif
                 updatehookmask(hL);
                 last_hook_call_in_c = false;
-            }
-            if (stepL == hL) {
-                if (step_mask & LUA_MASKRET) {
-                    step_hook_line(hL, ar);
+                if (update_mask) {
+                    update_hook(hL);
                 }
+                if (break_mask & LUA_MASKRET) {
+                    break_hook_return(hL, ar);
+                }
+                if (stepL == hL) {
+                    if (step_mask & LUA_MASKRET) {
+                        step_hook_line(hL, ar);
+                    }
+                }
+                else if (step_mask & LUA_MASKLINE) {
+                    // step in
+                    break;
+                }
+                return;
             }
 #endif
             break;

[thewhitegoatcb]

@fesily works perfectly, great job minor quirk if there's a code like this

c_func() and c_func()

and I put a breakpoint and hit continue after trigger, it will trigger again forgot to test if it happens with normal lua functions too just a minor thing

[fesily]

@fesily works perfectly, great job minor quirk if there's a code like this

c_func() and c_func()

and I put a breakpoint and hit continue after trigger, it will trigger again forgot to test if it happens with normal lua functions too just a minor thing

This temporarily do not deal with it, you can modify the pull

[thewhitegoatcb]

@fesily I think I have made a mistake while testing. Tested again yesterday, your proposed last fix doesn't seem to fix performance. but the previous one with is_step_mode does work

[fesily]

@fesily I think I have made a mistake while testing. Tested again yesterday, your proposed last fix doesn't seem to fix performance. but the previous one with is_step_mode does work

As long as it works

[fesily]

nogc64 this function you should submit a new pull

[thewhitegoatcb]

yes, my bad I should have moved it to a different branch

[fesily]

@actboy168 合并吧,这样就可以了