Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1673543002:
Split out fallback stack limit determination. (Closed)
|
Created:
4 years, 10 months ago by sof Modified:
4 years, 10 months ago CC:
chromium-reviews, oilpan-reviews, Mads Ager (chromium), blink-reviews, kinuko+watch, kouhei+heap_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit out fallback stack limit determination.
In case we're unable to get an estimate of the stack size for a platform
thread, a small stack size is assumed and its effective upper bound is
then probed and computed.
Do that in a separate method to avoid compiler-injected stack exhaustion checks
(MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit().
This also facilitates running GCs when up against stack pressure on Windows
platforms.
R=haraken
BUG=582819
Committed: https://crrev.com/19cfc3ef242d1af45f6d26115828db83db87b161
Cr-Commit-Position: refs/heads/master@{#373787}
Patch Set 1 #Patch Set 2 : rebased #
Total comments: 4
Patch Set 3 : comment improvements #
Messages
Total messages: 17 (8 generated)
Description was changed from ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, we fallback to assuming a small stack and compute the effective upper bound of that. Do that in a separate method so as to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). It also facilitates running GCs when up against stack pressure on Windows platforms. R= BUG=582819 ========== to ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, a small stack size is assumed and its effective upper bound is then probed and computed. Do that in a separate method to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). This also facilitates running GCs when up against stack pressure on Windows platforms. R= BUG=582819 ==========
sigbjornf@opera.com changed reviewers: + oilpan-reviews@chromium.org
please take a look. The reorg here _might_ allow GCs to run under high stack pressure on Windows. If not, then we additionally need something like https://codereview.chromium.org/1670093002/ .
haraken@chromium.org changed reviewers: + haraken@chromium.org
LGTM https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp (right): https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp:38: // Allocate a 32KB object on stack and query stack frame base after it. If we take this approach, I think we can increase the threshold. Shall we try 128 KB (for example) and see if we get crash reports?
https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp (right): https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp:38: // Allocate a 32KB object on stack and query stack frame base after it. On 2016/02/05 09:05:57, haraken wrote: > > If we take this approach, I think we can increase the threshold. Shall we try > 128 KB (for example) and see if we get crash reports? On Windows this method will never be executed, so that won't make much of a difference for the associated bug. i.e., the compiler will hoist & include _chkstk() calls in prologues if the method/function makes > 4k of local stack allocations -- the check isn't delayed until the stack pointer is actually bumped. Which it will never be on Windows where we can determine stack size reliably.
https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp (right): https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp:60: if (!stackSize) { It would be worth having a comment and mention on what platforms we fall back to the fallback stack size.
On 2016/02/05 09:13:02, sof wrote: > https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp (right): > > https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp:38: // Allocate a > 32KB object on stack and query stack frame base after it. > On 2016/02/05 09:05:57, haraken wrote: > > > > If we take this approach, I think we can increase the threshold. Shall we try > > 128 KB (for example) and see if we get crash reports? > > On Windows this method will never be executed, so that won't make much of a > difference for the associated bug. Makes sense.
https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp (right): https://codereview.chromium.org/1673543002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/heap/StackFrameDepth.cpp:60: if (!stackSize) { On 2016/02/05 09:19:02, haraken wrote: > > It would be worth having a comment and mention on what platforms we fall back to > the fallback stack size. Yes, added. Having a non-zero default estimate for pthreads-based platforms, is probably worth it. But not by this CL.
Description was changed from ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, a small stack size is assumed and its effective upper bound is then probed and computed. Do that in a separate method to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). This also facilitates running GCs when up against stack pressure on Windows platforms. R= BUG=582819 ========== to ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, a small stack size is assumed and its effective upper bound is then probed and computed. Do that in a separate method to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). This also facilitates running GCs when up against stack pressure on Windows platforms. R=haraken BUG=582819 ==========
The CQ bit was checked by sigbjornf@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/1673543002/#ps40001 (title: "comment improvements")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1673543002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1673543002/40001
Message was sent while issue was closed.
Description was changed from ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, a small stack size is assumed and its effective upper bound is then probed and computed. Do that in a separate method to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). This also facilitates running GCs when up against stack pressure on Windows platforms. R=haraken BUG=582819 ========== to ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, a small stack size is assumed and its effective upper bound is then probed and computed. Do that in a separate method to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). This also facilitates running GCs when up against stack pressure on Windows platforms. R=haraken BUG=582819 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, a small stack size is assumed and its effective upper bound is then probed and computed. Do that in a separate method to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). This also facilitates running GCs when up against stack pressure on Windows platforms. R=haraken BUG=582819 ========== to ========== Split out fallback stack limit determination. In case we're unable to get an estimate of the stack size for a platform thread, a small stack size is assumed and its effective upper bound is then probed and computed. Do that in a separate method to avoid compiler-injected stack exhaustion checks (MSVC's _chkstk()) from being always run for StackFrameDepth::enableStackLimit(). This also facilitates running GCs when up against stack pressure on Windows platforms. R=haraken BUG=582819 Committed: https://crrev.com/19cfc3ef242d1af45f6d26115828db83db87b161 Cr-Commit-Position: refs/heads/master@{#373787} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/19cfc3ef242d1af45f6d26115828db83db87b161 Cr-Commit-Position: refs/heads/master@{#373787} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
