[webkit-reviews] review denied: [Bug 128562] Adjust VM::stackLimit based on the size of the largest FTL stack produced : [Attachment 223803] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 128562: Adjust VM::stackLimit based on the size of the largest FTL stack
produced
https://bugs.webkit.org/show_bug.cgi?id=128562

Attachment 223803: Patch
https://bugs.webkit.org/attachment.cgi?id=223803&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223803&action=review


The current updateStackLimitWithReservedZoneSize() does 2 things:
1. update VM::m_reservedZoneSize.
2. Compute the new stack limit based on that reservedZoneSize.

I think you should rename your setStackLimit() to updateStackLimit() and make
the following changes:

1. Move the stack limit computation logic from
updateStackLimitWithReservedZoneSize() here, and have
updateStackLimitWithReservedZoneSize() only update m_reservedZoneSize, and
delegate to updateStackLimit() to compute the new stack limit.

2. Change VM::updateFTLLargestStack() to update VM::m_largestFTLStackSize, and
then delegate to updateStackSize() to compute the new stack limit.  nit: I
suggest renaming updateFTLLargestStack() to updateFTLLargestStackSize().

3. Change updateStackLimit() to not take a limit pointer.  Instead of biasing a
pointer that someone else passes in, it should compute the new stack limit
(i.e. m_stackLimit and m_ftlStackLimit) based on the m_largestFTLStackSize
m_reservedZoneSize, and Options::maxPerThreadStackUsage().

Instead of biasing the stack limit result returned from
StackBounds::recursionLimit(), add the needed FTL stack allowance to the needed
amount of reservedZoneSize and let StackBounds::recursionLimit() do the limit
computation.  

The role of StackBounds::recursionLimit() is solely to compute the stack limit
given the parameters you provide.  It doesn’t really care if you use the stack
for recursion or not.  Hence, the name can be a little misleading.  The
advantage of letting StackBounds::recursionLimit() do the stack limit
computation is that it handles underflow / overflow edge cases and properly
bounds the limit to the stack’s address range.

To summarize, my thinking of the division of labor goes as follows:

1. StackBounds::recursionLimit() knows about the real bounds of the stack, and
computes the stack limit given the various parameters that we pass to it.  It
will take care of underflows / overflows and ensure that the stack limit
pointer it computes is a sane address on the stack.

2. updateStackLimit() computes the new VM stack limit(s) by factoring in the
FTL stack size needs, the current reservedZoneSize, and
Options::maxPerThreadStackUsage().

3. updateFTLLargestStack() updates the FTL largest stack size and delegates to
updateStackLimit() to update the stack limit.

4. updateStackLimitWithReservedZoneSize() updates the reservedZoneSize and
delegates to updateStackLimit() to update the stack limit.  If you prefer, you
can rename it to updateReservedZoneSize() to be consistent with
updateFTLLargestStackSize().

> Source/JavaScriptCore/ChangeLog:9
> +	   compiled function. Added VM::m_FTLStackLimit for FTL stack limit. 
Changed

I think our convention is to name this m_ftlStackLimit instead of
m_FTLStackLimit.

> Source/JavaScriptCore/runtime/VM.cpp:226
> +    , m_FTLStackLimit(0)

Ditto,	use m_ftlStackLimit.