Re: [Tarantool-patches] [PATCH v2] lua: remove excess Lua call from table encoding

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

The patch looks ok after I figured out that you actually sink the pcall
down the callstack, keeping it only for the case __serialize is a
function, leaving all other cases as is.

What misled me is the changelog, where you mentioned you _removed_
protected frame:

  * Refactor Lua table encoding: removed excess Lua function object and
  protected Lua frame creation to improve `msgpack.encode()` performance.

I believe it'll be better to update the changelog descripton.


LGTM,

Sergos


On 05 июн 10:14, Sergey Kaplun wrote:
> Hi!
> 
> It's a little bit complicated to find a good test that shows the
> difference of behaviour. AFAIK there is only one case when we can face
> with OOM error or so on - reallocating of Lua stack. But it can happen
> with old version too (when we push cfunction, table or lightuserdata at
> Lua stack before pcall). Now instead three values we push at Lua stack
> only two (Lua function from metatable and table itself if serializing
> function exists) before pcall. So with this patch the probability of
> raising OOM error should be decreased.
> 
> Of course we can check it with an eror injection, but it can be catched
> with old behaviour too. It will be nice if you can provide any idea of
> this kind of test to check this.
> 
> On 02.06.20, Sergey Ostanevich wrote:
> > Hi!
> > 
> > Thanks for the patch!
> > 
> > Still I would like to see some tests - perhaps with errinj to emulate
> > OOM or some other case that trigger the 'excess protected frame' need.
> > So that after your changes it still passes.
> > 
> > Regards,
> > Sergos
> > 
> 
> -- 
> Best regards,
> Sergey Kaplun