[Oilpan] Set stack limit using estimated sizes

Set the stack limit size based on estimated sizes.

To check if the stack memory has enough room for recursive
calls, we tried to allocate a large object and assumed the size.
This CL estimate the real thread stack limit using APIs.
In some cases which such APIs do not work correctly, we set
the limit using the old algorithm.
(AFAIK ChromeOS and MacOSX need the old method.)


BUG=420515, 457982
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190239

[peria, 2015-02-10 06:41:46]

peria@chromium.org changed reviewers:
+ haraken@chromium.org, kouhei@chromium.org, oilpan-reviews@chromium.org

[peria, 2015-02-10 06:41:47]

PTL.

[haraken, 2015-02-10 08:53:22]

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
File Source/platform/heap/StackFrameDepth.cpp (left):

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:46: // in these platforms.

Update this comment instead of removing.

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:31: void
StackFrameDepth::configureLimit()

Can we avoid calling getUnderestimatedStackSize() and getStackStart() in each
configureLimit()? (configureLimit() is called at each marking.)

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:33: size_t stackSize =
getUnderestimatedStackSize();

Note: You can add RELEASE_ASSERT(stackSize >= 128 * 1024) and run various try
bots. Then you can understand on which platform/tests a reasonable stack size is
not available.

[peria, 2015-02-10 11:59:24]

Patchset #6 (id:100001) has been deleted

[peria, 2015-02-10 12:05:29]

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
File Source/platform/heap/StackFrameDepth.cpp (left):

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:46: // in these platforms.
On 2015/02/10 08:53:22, haraken wrote:
> 
> Update this comment instead of removing.

Done.

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:31: void
StackFrameDepth::configureLimit()
The instance of StackFrameDepth is globally unique instance, and StackFrames are
different on each thread.
So we have to set limit everytime on calling recursive functions.

To avoid calling configureLimit(), we have to make StackFrameDepth be a member
value of ThreadState. (TODO)

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:33: size_t stackSize =
getUnderestimatedStackSize();
On 2015/02/10 08:53:22, haraken wrote:
> 
> Note: You can add RELEASE_ASSERT(stackSize >= 128 * 1024) and run various try
> bots. Then you can understand on which platform/tests a reasonable stack size
is
> not available.

Acknowledged.

[haraken, 2015-02-10 12:15:09]

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:33: size_t stackSize =
getUnderestimatedStackSize();
On 2015/02/10 08:53:22, haraken wrote:
> 
> Note: You can add RELEASE_ASSERT(stackSize >= 128 * 1024) and run various try
> bots. Then you can understand on which platform/tests a reasonable stack size
is
> not available.

Would you try this before landing this CL? We might want to understand on which
platform we can get the stack size and we can make the system workable as
expected.

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:20: const int
StackFrameDepth::kStackRoomSize = 1024;

Shall we make this a static variable of StackFrameDepth::configureLimit()?

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:46: dummy[0] = 0;

What is this change for?

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:52: // We cannot get the stack size on
the main thread in these platforms, because

Update the comment like:

// pthread_getattr_np() can fail if the thread is not invoked by
pthread_create() (e.g., the main thread of webkit_unit_tests). In this case,
this method returns 0 and the caller must handle it.

[kouhei (in TOK), 2015-02-10 12:35:38]

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:20: const int
StackFrameDepth::kStackRoomSize = 1024;
On 2015/02/10 12:15:09, haraken wrote:
> 
> Shall we make this a static variable of StackFrameDepth::configureLimit()?

static const int +1

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:46: dummy[0] = 0;
On 2015/02/10 12:15:09, haraken wrote:
> 
> What is this change for?

To check if the top of the stack is actually writable, [0] was correct, as
[sizeof(dummy) -1] is actually at bottom of the stack when stack grow toward
lower address.
Please add it to CL description and/or separate CL.

[peria, 2015-02-12 02:31:58]

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/80001/Source/platform/heap/Sta...
Source/platform/heap/StackFrameDepth.cpp:33: size_t stackSize =
getUnderestimatedStackSize();
On 2015/02/10 12:15:09, haraken wrote:
> On 2015/02/10 08:53:22, haraken wrote:
> > 
> > Note: You can add RELEASE_ASSERT(stackSize >= 128 * 1024) and run various
try
> > bots. Then you can understand on which platform/tests a reasonable stack
size
> is
> > not available.
> 
> Would you try this before landing this CL? We might want to understand on
which
> platform we can get the stack size and we can make the system workable as
> expected.

Trying in https://codereview.chromium.org/918883003/

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:20: const int
StackFrameDepth::kStackRoomSize = 1024;
On 2015/02/10 12:35:38, kouhei wrote:
> On 2015/02/10 12:15:09, haraken wrote:
> > 
> > Shall we make this a static variable of StackFrameDepth::configureLimit()?
> 
> static const int +1

Done.

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:46: dummy[0] = 0;
On 2015/02/10 12:35:38, kouhei wrote:
> On 2015/02/10 12:15:09, haraken wrote:
> > 
> > What is this change for?
> 
> To check if the top of the stack is actually writable, [0] was correct, as
> [sizeof(dummy) -1] is actually at bottom of the stack when stack grow toward
> lower address.
> Please add it to CL description and/or separate CL.

will do in another CL.

https://codereview.chromium.org/910663002/diff/120001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:52: // We cannot get the stack size on
the main thread in these platforms, because
On 2015/02/10 12:15:09, haraken wrote:
> 
> Update the comment like:
> 
> // pthread_getattr_np() can fail if the thread is not invoked by
> pthread_create() (e.g., the main thread of webkit_unit_tests). In this case,
> this method returns 0 and the caller must handle it.

Done.

[kouhei (in TOK), 2015-02-12 03:46:05]

lgtm

https://codereview.chromium.org/910663002/diff/140001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/140001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:32: static const int kStackRoomSize=
1024;
space

[kouhei (in TOK), 2015-02-12 03:47:52]

Also, please summarize the change in CL description.

[peria, 2015-02-12 04:12:03]

New patchsets have been uploaded after l-g-t-m from kouhei@chromium.org

[peria, 2015-02-16 07:47:00]

I checked the behavior in https://codereview.chromium.org/918883003/
and found ChromeOS and MacOSX needs more investigation.

PTAL to commit it.

[haraken, 2015-02-16 08:15:24]

LGTM

https://codereview.chromium.org/910663002/diff/200001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/200001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:50: size_t
StackFrameDepth::getUnderestimatedStackSize()

Add a FIXME about MacOSX and ChromiumOS.

[peria, 2015-02-16 08:26:31]

New patchsets have been uploaded after l-g-t-m from haraken@chromium.org

[peria, 2015-02-16 08:44:23]

The CQ bit was checked by peria@chromium.org

[peria, 2015-02-16 08:45:17]

https://codereview.chromium.org/910663002/diff/200001/Source/platform/heap/St...
File Source/platform/heap/StackFrameDepth.cpp (right):

https://codereview.chromium.org/910663002/diff/200001/Source/platform/heap/St...
Source/platform/heap/StackFrameDepth.cpp:50: size_t
StackFrameDepth::getUnderestimatedStackSize()
On 2015/02/16 08:15:24, haraken wrote:
> 
> Add a FIXME about MacOSX and ChromiumOS.

Done.

[commit-bot: I haz the power, 2015-02-16 08:45:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910663002/220001

[commit-bot: I haz the power, 2015-02-16 10:03:29]

Committed patchset #11 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190239

[kouhei (in TOK), 2015-02-16 10:08:31]

still lgtm

[tkent, 2015-02-16 10:40:44]

A revert of this CL (patchset #11 id:220001) has been created in
https://codereview.chromium.org/921083003/ by tkent@chromium.org.

The reason for reverting is: Broke HeapTest.TraceDeepEagerly.

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/44281
.

[peria, 2015-02-16 12:58:38]

lgtm

I had to expand that loop, because we can go deeper eagerly.