[webkit-reviews] review denied: [Bug 26276] Need a mechanism to determine stack extent : [Attachment 66474] interpreteroverflow.diff

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 26276: Need a mechanism to determine stack extent
https://bugs.webkit.org/show_bug.cgi?id=26276

Attachment 66474: interpreteroverflow.diff
https://bugs.webkit.org/attachment.cgi?id=66474&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
Hey xan,

Out of coincidence I've hit a similar issue, I need to improve the
bytecompiler's recursion guards.  As such I think we could do with some more of
this code in a common place, where it can be shared.

I'm going to give this an r- for now, since I think this change can be much
simpler when my patch lands – you can basically just call
globalData->stack().recursionCheck().  However I think in making this change we
should also address one question slightly more thoroughly – how much machine
stack space so we need to nest iterations of the VM? (the new recursionCheck()
method defaults to checking that 4k of stack is available, but a different
limit can be specified, and nesting entries into Interpreter::execute might
take more than this – with the interpreter enabled the frame size for
privateExecute is large).

I'd suggest we should perform a couple of quick empirical tests to set a
sensible value for the minimum amount of space to require upon entry into
Interpreter::execute.  We could write some test code that causes nested entries
into JIT code & into private execute (we should test with the interpreter too),
and measure the difference in the stack position at these points.  This may
give us some guide as the to depth of stack space needed, but I guess this may
be flawed – this may not be representative of a use case that uses the most
stack space.

A better experiment might be to write a tool to dump the processes vm map on
exit, run some test cases, and look at how much of the stack has been mapped in
resident (we could do this on OS X using vmmap) - this would be harder, but
would give us a better indication of common stack usage in JSC.  Maybe there is
an easier way to produce similar information.

What do you think? - do you think you can come up with some form of test to
pick a sensible amount of stack space to require to permit reentry?  If not, I
suggest we at least hard code a constant to demand a more significant chunk of
stack space be available to permit reentry, say, passing 16k/32k to
recursionCheck().

cheers,
G.