Chromium Code Reviews
Help | Chromium Project | Sign in
(764)

Issue 242014: Adds an API for setting the stack limit per-thread. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 7 months ago by dominicc
Modified:
2 years, 11 months ago
Reviewers:
Erik Corry
Visibility:
Public.

Description

Adds an API for setting the stack limit per-thread.

This fixes bug 61 and bug 442.

This proposed change has been deprecated in favor of fixing the SetResourceConstraints API.

Patch Set 1 #

Patch Set 2 : Adds a unit test. #

Total comments: 1

Patch Set 3 : Fixes a bug when an interrupt resets a limit with no active stack guards. #

Patch Set 4 : Sets the stack limit after the ARM simulator has initialized its stack area. #

Patch Set 5 : Cosmetic changes to minimize the diff. #

Total comments: 15

Patch Set 6 : Changes in response to code review feedback. #

Patch Set 7 : Leaves the limits of pending interrupts alone instead of reestablishing them." #

Unified diffs Side-by-side diffs Delta from patch set Stats (+167 lines, -93 lines) Lint Patch
M include/v8.h View 1 chunk +11 lines, -6 lines 0 comments 0 errors Download
M src/api.cc View 1 chunk +10 lines, -12 lines 0 comments 0 errors Download
M src/arm/simulator-arm.h View 2 chunks +6 lines, -7 lines 0 comments 0 errors Download
M src/execution.h View 1 2 3 4 5 3 chunks +24 lines, -9 lines 0 comments 0 errors Download
M src/execution.cc View 1 2 3 4 5 6 10 chunks +47 lines, -32 lines 0 comments ? errors Download
M src/ia32/simulator-ia32.h View 1 chunk +5 lines, -9 lines 0 comments 0 errors Download
M src/v8threads.cc View 1 chunk +2 lines, -0 lines 0 comments 0 errors Download
M src/x64/simulator-x64.h View 1 chunk +5 lines, -9 lines 0 comments 0 errors Download
M test/cctest/test-api.cc View 1 2 3 7 chunks +57 lines, -9 lines 0 comments 7 errors Download
Trybot results:
Commit:

Messages

Total messages: 3
dominicc_google.com
http://codereview.chromium.org/242014/diff/2001/3007 File src/v8threads.cc (right): http://codereview.chromium.org/242014/diff/2001/3007#newcode142 Line 142: // This is a new thread. I could ...
4 years, 7 months ago #1
Erik Corry
I'm afraid I'm not very happy with this change. Perhaps I'm just not getting it. ...
4 years, 6 months ago #2
dominicc_google.com
4 years, 6 months ago #3
Comments inline.

http://codereview.chromium.org/242014/diff/9001/8014
File src/execution.cc (right):

http://codereview.chromium.org/242014/diff/9001/8014#newcode236
Line 236: thread_local_.interrupt_flags_ != 0));
On 2009/09/28 14:10:15, Erik Corry wrote:
> I think these asserts and the corresponding comments should just go away.  Why
> should we not be allowed to create a StackGuard object if the stack limits
have
> already been set?  Instead I think we should do nothing and ensure the
> destructor does nothing if the stack limits were already set.

This moves a little further away from the original design. We could get this
change in to unblock fixing the worker bug and file a clean-up bug to work on
redesigning StackGuard.

http://codereview.chromium.org/242014/diff/9001/8014#newcode247
Line 247: // up to address 0x00000000.
On 2009/09/28 14:10:15, Erik Corry wrote:
> This comment makes no sense (I know you didn't write it).

It is a little awkward--it is guarding against underflow, not overflow; and it
is commenting on a situation where a machine puts its thread segments
sufficiently close to 0x0, but not quite all the way there, which is probably
rare. I've revised it.

http://codereview.chromium.org/242014/diff/9001/8014#newcode249
Line 249: reinterpret_cast<uintptr_t>(this) >= kLimitSize ?
On 2009/09/28 14:10:15, Erik Corry wrote:
> Here we are comparing the current stack address with a small numeric constant.

> That makes no sense at all to me.

This is an underflow check; this is existing code. At least after this change it
in *one* place instead of four (here plus ARM, ia32 and x64 simulator includes.)

http://codereview.chromium.org/242014/diff/9001/8014#newcode257
Line 257: set_limits(kInterruptLimit, access);
On 2009/09/28 14:10:15, Erik Corry wrote:
> Not sure why these lines are here (I know you didn't write them).  If the
> interrupt flags are already set then surely the limits would already be set to
> kInterruptLimit.

Yes, I agree this existing code looks dubious, or is at least complex. I've done
some debugging, I think this is just re-establishing the limit for any "pending"
interrupt since the last set of StackGuards was torn down. I've uploaded an
alternative, (although its behavior between sets of StackGuards is slightly
different) that might illustrate what's happening here--see the change to
StackGuard::~StackGuard in patch set 7.

I doubt that this is really related to what I'm trying to implement here; I
think changing it is clean-up orthogonal to this change.

http://codereview.chromium.org/242014/diff/9001/8014#newcode274
Line 274: if (--thread_local_.nesting_ == 0) {
On 2009/09/28 14:10:15, Erik Corry wrote:
> Instead of looking at the nesting limit we should just restore the old values
if
> we set the limit when the object was created.

This resets the limit in use (jslimit_, climit_) and not the limit that *will*
be used when the stack guard is active (initial_jslimit_, initial_climit_).

I think the intent of the original code is to be able to configure a limit in
initial_*_, but still be able to tell whether the stack guard is active,
inactive (kIllegalLimit) or running an interrupt (kInterruptLimit.)

http://codereview.chromium.org/242014/diff/9001/8014#newcode300
Line 300: if (thread_local_.nesting_ > 0 &&
On 2009/09/28 14:10:15, Erik Corry wrote:
> Could we have a comment explaining why this should only work if the nesting is
>
> 0?

Done.

http://codereview.chromium.org/242014/diff/9001/8014#newcode421
Line 421: ThreadLocal blank;
On 2009/09/28 14:10:15, Erik Corry wrote:
> Does this mean that new threads per default have no stack limit?  I think I
> would prefer if they had a default stack limit of 1Mbyte or something.

New threads have a default stack limit kLimitSize from their first StackGuard.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6