Bug 128562 – Adjust VM::stackLimit based on the size of the largest FTL stack produced

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

We need to adjust the stack limit use for stack overflow check to take into account that an FTL function can be called and it may access the stack before it has done a stack check.  We can keep track of the size of the largest stack from and FTL function and use that as an adjustment to VM::stackLimit.  Given that an FTL function can all another FTL function we need use 2x the largest FTL stack size.

[Michael Saboff]

Created attachment 223803 [details]
Patch

[WebKit Commit Bot]

Attachment 223803 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/VM.h:514:  The parameter name "limit" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 223803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223803&action=review

> Source/JavaScriptCore/runtime/VM.h:540
>              void* m_stackLimit;
>              void* m_jsStackLimit;
>          };
> +#   if ENABLE(FTL_JIT)
> +        void* m_FTLStackLimit;

Why introduce a new stack limit data member instead of using the existing one?

[Michael Saboff]

(In reply to comment #3)
> (From update of attachment 223803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223803&action=review
> 
> > Source/JavaScriptCore/runtime/VM.h:540
> >              void* m_stackLimit;
> >              void* m_jsStackLimit;
> >          };
> > +#   if ENABLE(FTL_JIT)
> > +        void* m_FTLStackLimit;
> 
> Why introduce a new stack limit data member instead of using the existing one?

The original m_stackLimit will be offset by m_largestFTLStackSize, while m_FTLStackLimit will be biased by 2 * m_FTLStackLimit.  We'll only use m_FTLStackLimit for FTL functions.

[Build Bot]

Comment on attachment 223803 [details]
Patch

Attachment 223803 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5468086811492352

New failing tests:
animations/stop-animation-on-suspend.html

[Build Bot]

Created attachment 223812 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Mark Lam]

Comment on attachment 223803 [details]
Patch

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.

[Michael Saboff]

Created attachment 223966 [details]
Patch with suggested updates

[Mark Lam]

Comment on attachment 223966 [details]
Patch with suggested updates

View in context: https://bugs.webkit.org/attachment.cgi?id=223966&action=review

r=me.

> Source/JavaScriptCore/runtime/VM.h:542
> +#   if ENABLE(FTL_JIT)
> +        void* m_ftlStackLimit;
> +        size_t m_largestFTLStackSize;
> +#   endif

Remove the spaces after the #.

[Michael Saboff]

Committed r163964: <http://trac.webkit.org/changeset/163964>