lua-users home
lua-l archive

[Date Prev][Date Next][Thread Prev][Thread Next] [Date Index] [Thread Index]


Hi Joseph

Compliments for the idea and the implementation.

I take a glace at the patch.

file ldo.c, function rethook, starting at line 337/340:

if (globalhook || (L->hookmask & LUA_MASKRET)) { /* is return hook on? */
...
  luaD_hook(L, globalhook, LUA_HOOKRET, -1, ftransfer, nres); /* call it */
  if (L->hookmask & LUA_MASKRET)
     luaD_hook(L, L->hook, LUA_HOOKRET, -1, ftransfer, nres); /* call it */


Looking at the indentation and the previous test,
I expected the globalhook is called this way:

  if (globalhook)
     luaD_hook(L, globalhook, LUA_HOOKRET, -1, ftransfer, nres); /* call it */



M


On Wed, 20 May 2020 at 04:09, Joseph C. Sible <josephcsible@gmail.com> wrote:
On Tue, May 19, 2020 at 8:21 AM Roberto Ierusalimschy
<roberto@inf.puc-rio.br> wrote:
>
> > Under the assumption that function pointers are atomic, perhaps
> > there's a way to fix the CallInfo race, the Undefined Behavior with C
> > threads, and being unable to stop a coroutine, all at once: add the
> > concept of a "global hook". Here's how I think it could work:
> >
> > * Create a new function called "lua_noophook" that just returns
> > without doing anything.
> > * Add a single "volatile" function pointer to global_State called
> > "globalhook", that points to lua_noophook by default.
> > * Everywhere that we check to run hooks now, also unconditionally call
> > the "G(L)->globalhook" function pointer, as if it were a LUA_MASKCOUNT
> > hook with a count of 1.
> >
> > I think that would have the desired semantics in almost every case
> > today that currently sets a hook asynchronously. (And in the uncommon
> > case of wanting to do something else, you could do it from the global
> > hook, and then reset it back to lua_noophook.)
> >
> > Does this seem like a good solution?
>
> Any estimates of the runtime cost of this implementation?
>
> -- Roberto

I implemented my idea, attached as a patch and also viewable here:
https://github.com/josephcsible/lua/commit/0d83524122ba09467fcade6f66fbfdef67fe5d39

I also switched the standalone lua binary's SIGINT hook to use the new
global hook, so as a quick test, you can run this in it:

    coroutine.wrap(function() while true do end end)()

And you'll now be able to interrupt that with a single Ctrl+C.

As a very crude benchmark, I ran ./all 20 times without my patch and
20 times with it, and compared the time.txt outputs. Bizarrely, the
average runtime was slightly lower with my patch. I don't think my
patch actually made it faster, but rather just that the minuscule
extra slowness got completely buried by noise.

If someone has a better Lua benchmark, please try it and share the results.

Also, let me know if there's any improvements I can make to this patch
or anything I can do to increase this feature's chances of being
incorporated in the official Lua release.

Joseph C. Sible