Stack guards are too conservative
Created attachment 199616 [details] Patch
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.
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.
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.
> 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.
(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.
(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.
Created attachment 199732 [details] Patch
<rdar://13098540>
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?
Committed r149136: <http://trac.webkit.org/changeset/149136>
Reopening to attach new patch.
Created attachment 199753 [details] Patch
Comment on attachment 199753 [details] Patch r=me
Committed r149146: <http://trac.webkit.org/changeset/149146>