[Isolates]...

[Isolates] Stack guards - static.

- Begin removing static state from stack guards.
- Split default isolate initialization into two phases:
    * Allocate all previously static objects
    * Call initialize methods on those static objects
  This is necessary to preserve the old behavior of the API; without the
  change some tests fail (where the failures cannot be solved by a call
  to V8::Initialize).


Committed: http://code.google.com/p/v8/source/detail?r=4832

[zarko, 2010-06-09 01:08:23]

From initial feedback, I had:

1. make StackGuard a real object.
2. SetResourceConstraints should be called before Initialize
3. Serializer tests can just call Initialize

There's still some trickiness regarding 2, 3; I've left the static global
isolate initializer in for the moment, but this is not meant to be a final
CL.

[Vitaly Repeshko, 2010-06-09 18:48:54]

LG except for the way StackGuard sets limits on Heap.


-- Vitaly

http://codereview.chromium.org/2715004/diff/36002/42005
File src/debug.h (right):

http://codereview.chromium.org/2715004/diff/36002/42005#newcode783
src/debug.h:783: Isolate* isolate = Isolate::Current();
Add TODO(isolates) to check this is the same isolate as in the constructor.

http://codereview.chromium.org/2715004/diff/36002/42006
File src/execution.cc (right):

http://codereview.chromium.org/2715004/diff/36002/42006#newcode46
src/execution.cc:46: // ClearThreadLocal(&thread_local_) is called by Isolate
once it is safe.
Looks really unfortunate. I think it should be safe provided Heap is initialized
before StackGuard.

http://codereview.chromium.org/2715004/diff/36002/42006#newcode336
src/execution.cc:336: ClearThreadLocal(&blank);
Can we decouple clearing the thread local from setting limits in the isolate?
The current situation looks too complicated and the comment describing its
previous state doesn't really help.

http://codereview.chromium.org/2715004/diff/36002/42007
File src/execution.h (right):

http://codereview.chromium.org/2715004/diff/36002/42007#newcode204
src/execution.h:204: // TODO(isolates): Technically this could be calculated
directly from
Move this next to other fields of this class.

http://codereview.chromium.org/2715004/diff/36002/42008
File src/heap.cc (right):

http://codereview.chromium.org/2715004/diff/36002/42008#newcode3947
src/heap.cc:3947: ASSERT(isolate_ != NULL);
Also check it's the current one?

http://codereview.chromium.org/2715004/diff/36002/42012
File src/isolate.h (right):

http://codereview.chromium.org/2715004/diff/36002/42012#newcode66
src/isolate.h:66: static bool
SetDefaultThreadResourceConstraints(ResourceConstraints* rc);
Never defined?

http://codereview.chromium.org/2715004/diff/36002/42012#newcode89
src/isolate.h:89: StackGuard* stack_guard() {
nit: Fit on one line.

http://codereview.chromium.org/2715004/diff/36002/42012#newcode155
src/isolate.h:155: void StackGuard::set_interrupt_limits(const ExecutionAccess&
lock) {
I think this function and the next one are okay to move to execution.cc (where
we can freely include the isolate header).

[zarko, 2010-06-09 20:30:05]

Revisions applied. Thanks for the feedback.

http://codereview.chromium.org/2715004/diff/36002/42006
File src/execution.cc (right):

http://codereview.chromium.org/2715004/diff/36002/42006#newcode46
src/execution.cc:46: // ClearThreadLocal(&thread_local_) is called by Isolate
once it is safe.
On 2010/06/09 18:48:54, Vitaly wrote:
> Looks really unfortunate. I think it should be safe provided Heap is
initialized
> before StackGuard.

StackGuard is inlined in Isolate. The global_isolate pointer is not initialized
until ::PreInit is called, which happens after the Isolate is constructed. 'it
is safe' means that the isolate under construction is inside global_isolate.

Removing the SetStackLimits() side-effect from Clear()/Initialize() makes this
behavior explicit.

http://codereview.chromium.org/2715004/diff/36002/42006#newcode336
src/execution.cc:336: ClearThreadLocal(&blank);
On 2010/06/09 18:48:54, Vitaly wrote:
> Can we decouple clearing the thread local from setting limits in the isolate?
> The current situation looks too complicated and the comment describing its
> previous state doesn't really help.

Yes-- I agree with doing this. In the CL I was trying to avoid changing
semantics where possible, hence the ugliness. I'll split out the limit call from
calls to the ctor.

[Vitaly Repeshko, 2010-06-09 21:22:37]

LGTM. Thanks for doing the refactoring work.


-- Vitaly

http://codereview.chromium.org/2715004/diff/38002/40007
File src/execution.cc (right):

http://codereview.chromium.org/2715004/diff/38002/40007#newcode350
src/execution.cc:350: ThreadLocal blank;
Can we make its constructor call Clear()?

http://codereview.chromium.org/2715004/diff/38002/40007#newcode426
src/execution.cc:426: if (thread_local_.Initialize()) {
isolate_->heap()->SetStackLimits(); }
nit: No {} for one-line ifs.

http://codereview.chromium.org/2715004/diff/38002/40007#newcode624
src/execution.cc:624: // Perform preemption.
Unrelated to your change, but this looks like a race when debugger support is
not enabled. We read the undefined value before unlocker is destructed. Please
put it in a {} block.

http://codereview.chromium.org/2715004/diff/38002/40008
File src/execution.h (right):

http://codereview.chromium.org/2715004/diff/38002/40008#newcode204
src/execution.h:204: Isolate* isolate_; // TODO(isolates): Technically this
could be calculated
The comment was formatted correctly. I meant that the field itself (and its
comment) should be moved to be next to other fields of this class (to line 270).