Bug 128562

Created attachment 223803 [details] Patch

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.

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?

(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.

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

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

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.

Created attachment 223966 [details] Patch with suggested updates

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 #.

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