Bug 115147 – Stack guards are too conservative

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Oliver Hunt]

Stack guards are too conservative

[Oliver Hunt]

Created attachment 199616 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/interpreter/Interpreter.cpp']" exit_code: 1
Source/JavaScriptCore/interpreter/Interpreter.cpp:122:  DEFAULT_REQUIRED_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/JavaScriptCore/interpreter/Interpreter.cpp:123:  DEFAULT_MINIMUM_USEABLE_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 2 files


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

[Geoffrey Garen]

Comment on attachment 199616 [details]
Patch

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

Would be good to explain what motivated these new limits, so we don't make the same mistake twice.

>> Source/JavaScriptCore/interpreter/Interpreter.cpp:122
>> +    const size_t DEFAULT_REQUIRED_STACK = 256 * 1024;
> 
> DEFAULT_REQUIRED_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

I think we should just remove DEFAULT_REQUIRED_STACK entirely -- it's very weird to have two different minimum values. In truth, the smaller minimum is the bottom line.

[Mark Lam]

Comment on attachment 199616 [details]
Patch

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

>>> Source/JavaScriptCore/interpreter/Interpreter.cpp:122
>>> +    const size_t DEFAULT_REQUIRED_STACK = 256 * 1024;
>> 
>> DEFAULT_REQUIRED_STACK is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> 
> I think we should just remove DEFAULT_REQUIRED_STACK entirely -- it's very weird to have two different minimum values. In truth, the smaller minimum is the bottom line.

Just to clarify, DEFAULT_REQUIRED_STACK was calculated based on an estimated worst case recursion in the rendering / layout path.  Unfortunately, I did not retain the details of that calculation, and the details have long been purged from my memory.  The purpose of DEFAULT_REQUIRED_STACK is so that JS code does not recurse so deeply that there isn’t room left for the native code to do its worst case recursion.  Hence, its value can be quite large because of the possible deep recursion in native code (WebCore side). 

And because DEFAULT_REQUIRED_STACK can be quite large, we need to make sure JS code gets at least some sane minimum amount of stack space it is allowed to run in ... this is in case the total physical stack size is not much bigger or even less than DEFAULT_REQUIRED_STACK.  Hence, DEFAULT_MINIMUM_USEABLE_STACK is there to provide for JS code’s needs.

Now, if we have stack checks in native code so that we can detect imminent stack overflows before it recurses too deep, then we can significantly reduce DEFAULT_REQUIRED_STACK at that time, and perhaps even get rid of it as Geoff suggested.  DEFAULT_REQUIRED_STACK only needs to be the worst case stack usage between 2 consecutive native stack checks (be it between 2 entries into the interpreter, between 2 stack check points in recursing native code, or between entry into the interpreter and the nearest stack check point in native code).

But for now, since we don’t have native stack checks in WebCore where deep recursion can occur, reducing DEFAULT_REQUIRED_STACK effectively increases the probability that native code can overflow the stack ... not that what we had a full proof guard against this before.  Anyway, just FYI, so you'll understand what these values mean.

[Geoffrey Garen]

> Just to clarify, DEFAULT_REQUIRED_STACK was calculated based on an estimated worst case recursion in the rendering / layout path.

Yes, I understand that.

My criticism is that, when DEFAULT_REQUIRED_STACK is larger than the stack's size, the code tries again at DEFAULT_MINIMUM_USEABLE_STACK. This means that DEFAULT_MINIMUM_USEABLE_STACK is the only true limit.

[Mark Lam]

(In reply to comment #5)
> > Just to clarify, DEFAULT_REQUIRED_STACK was calculated based on an estimated worst case recursion in the rendering / layout path.
> 
> Yes, I understand that.
> 
> My criticism is that, when DEFAULT_REQUIRED_STACK is larger than the stack's size, the code tries again at DEFAULT_MINIMUM_USEABLE_STACK. This means that DEFAULT_MINIMUM_USEABLE_STACK is the only true limit.

That’s only always true.  Let me illustrate with some specific examples:

Using the original values of DEFAULT_REQUIRED_STACK of 1M and DEFAULT_MINIMUM_USEABLE_STACK of 128K, here are some scenarios:


    Stack size  Stack Limit
1. 128K         128K           // DEFAULT_MINIMUM_USEABLE_STACK takes effect
2. 256K         128K           // DEFAULT_MINIMUM_USEABLE_STACK takes effect
3. 1M            128K           // DEFAULT_MINIMUM_USEABLE_STACK takes effect
4. 2M            1.875M       // DEFAULT_REQUIRED_STACK  takes effect
5. 3M            2.875M       // DEFAULT_REQUIRED_STACK  takes effect

DEFAULT_REQUIRED_STACK allows us to use more stack space if its available.  Are you suggesting that we should limit stack usage to only DEFAULT_MINIMUM_USEABLE_STACK even if we have more stack space available?  I don’t agree with that.  As a platform, if the user configures the stack size to be large, I think we should allow them to use it.  The minimum value is needed only because DEFAULT_REQUIRED_STACK is very conservative.

The ideal situation would be to have native stack check points in place, and being able to set a DEFAULT_REQUIRED_STACK that is more reasonable (presumably in 10s if Ks).  When we have that, then we can do away with DEFAULT_MINIMUM_USEABLE_STACK altogether.

[Oliver Hunt]

(In reply to comment #6)
> (In reply to comment #5)
> > > Just to clarify, DEFAULT_REQUIRED_STACK was calculated based on an estimated worst case recursion in the rendering / layout path.
> > 
> > Yes, I understand that.
> > 
> > My criticism is that, when DEFAULT_REQUIRED_STACK is larger than the stack's size, the code tries again at DEFAULT_MINIMUM_USEABLE_STACK. This means that DEFAULT_MINIMUM_USEABLE_STACK is the only true limit.
> 
> That’s only always true.  Let me illustrate with some specific examples:
> 
> Using the original values of DEFAULT_REQUIRED_STACK of 1M and DEFAULT_MINIMUM_USEABLE_STACK of 128K, here are some scenarios:
> 
> 
>     Stack size  Stack Limit
> 1. 128K         128K           // DEFAULT_MINIMUM_USEABLE_STACK takes effect
> 2. 256K         128K           // DEFAULT_MINIMUM_USEABLE_STACK takes effect
> 3. 1M            128K           // DEFAULT_MINIMUM_USEABLE_STACK takes effect
> 4. 2M            1.875M       // DEFAULT_REQUIRED_STACK  takes effect
> 5. 3M            2.875M       // DEFAULT_REQUIRED_STACK  takes effect
> 
> DEFAULT_REQUIRED_STACK allows us to use more stack space if its available.  Are you suggesting that we should limit stack usage to only DEFAULT_MINIMUM_USEABLE_STACK even if we have more stack space available?  I don’t agree with that.  As a platform, if the user configures the stack size to be large, I think we should allow them to use it.  The minimum value is needed only because DEFAULT_REQUIRED_STACK is very conservative.
> 
> The ideal situation would be to have native stack check points in place, and being able to set a DEFAULT_REQUIRED_STACK that is more reasonable (presumably in 10s if Ks).  When we have that, then we can do away with DEFAULT_MINIMUM_USEABLE_STACK altogether.

Okay, i find this code incredibly hard to follow.

The _only_ thing that matters is the amount of stack left, nothing else.  The current terminology is inverted from what makes sense.

[Oliver Hunt]

Created attachment 199732 [details]
Patch

[Oliver Hunt]

<rdar://13098540>

[Geoffrey Garen]

Comment on attachment 199732 [details]
Patch

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

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:128
> +    // These sizes were derived from the stack usage of a number of sites when
> +    // layout occurs when we've already consumed most of the C stack.

I thought we just measured the stack usage of layout in WebCore?

[Oliver Hunt]

Committed r149136: <http://trac.webkit.org/changeset/149136>

[Oliver Hunt]

Reopening to attach new patch.

[Oliver Hunt]

Created attachment 199753 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 199753 [details]
Patch

r=me

[Oliver Hunt]

Committed r149146: <http://trac.webkit.org/changeset/149146>