Per-isolate v8::Locker and v8::Unlocker

This implements per-isolate locking and unlocking, including tests

BUG=
TEST=

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

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

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

[Dmitry Lomov, 2011-04-13 00:22:45]

This implements per-isolate Locker and Unlocker, and covers them with
unit-tests.

[Dmitry Titov, 2011-04-13 00:45:10]

Drive-by idea:

http://codereview.chromium.org/6788023/diff/11001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3408
include/v8.h:3408: explicit Locker(Isolate* isolate);
Consider: If it was:
explicit Locker(Isolate* isolate = NULL);
then there would be only 1 ctor, 1 IsLocked and no Init() method in this
API-specifying file... Same for Unlocker.
See TerminateExecution() in this file for precedent.

[Dmitry Lomov, 2011-04-13 00:54:12]

http://codereview.chromium.org/6788023/diff/11001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3408
include/v8.h:3408: explicit Locker(Isolate* isolate);
Yes, this sounds reasonable to me
On 2011/04/13 00:45:10, Dmitry Titov wrote:
> Consider: If it was:
> explicit Locker(Isolate* isolate = NULL);
> then there would be only 1 ctor, 1 IsLocked and no Init() method in this
> API-specifying file... Same for Unlocker.
> See TerminateExecution() in this file for precedent.

[Vitaly Repeshko, 2011-04-15 00:29:39]

First round of comments.

http://codereview.chromium.org/6788023/diff/11001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3316
include/v8.h:3316: * Multiple threads in V8 are allowed, but only one thread at
a time
This comment needs to be updated to make it clear that using lockers is actually
required for multi-threaded access and we also need some examples with isolates.

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3432
include/v8.h:3432: * Returns whether or not the locker for a given isolate is
locked by the current thread.
nit: Long line.

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3434
include/v8.h:3434: static bool IsLocked(Isolate * isolate);
nit: Drop space before '*'.

http://codereview.chromium.org/6788023/diff/11001/src/execution.cc
File src/execution.cc (right):

http://codereview.chromium.org/6788023/diff/11001/src/execution.cc#newcode461
src/execution.cc:461: isolate_->FindOrAllocatePerThreadDataForThisThread();
Are you sure you need "OrAllocate" here?

http://codereview.chromium.org/6788023/diff/11001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6788023/diff/11001/src/heap.h#newcode1941
src/heap.h:1941: explicit DisallowAllocationFailure(Heap* heap) : heap_(heap) {
Please don't waste time optimizing debug-only facilities.

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc
File src/isolate.cc (left):

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc#oldcode643
src/isolate.cc:643: ASSERT(Isolate::Current() == this);
Removing this is dangerous (at least for now), because passing and caching the
isolate pointer is an optimization and some code may still use
Isolate::Current() with hard to predict effects. Can you just set it in the
thread local slot like it's done in the de-initialization code? I know it's
ugly, but it's safer.

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc#newcode312
src/isolate.cc:312: 
nit: Two blank lines between functions.

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc#newcode323
src/isolate.cc:323: void Isolate::EnsurePreinitializedForThisThread() {
ForThisThread doesn't make much sense, since the isolate can only be
preinitialized once. How about LockedPreInit?

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc#newcode324
src/isolate.cc:324: ScopedLock lock(process_wide_mutex_);
I don't think we really need a process-wide mutex here. You could ensure the
mutex from the thread manager is already available and use it here.

http://codereview.chromium.org/6788023/diff/11001/src/stub-cache.cc
File src/stub-cache.cc (left):

http://codereview.chromium.org/6788023/diff/11001/src/stub-cache.cc#oldcode45
src/stub-cache.cc:45: ASSERT(isolate == Isolate::Current());
Please restore this (see my comment in isolate.cc).

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc
File src/v8threads.cc (right):

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode48
src/v8threads.cc:48: : has_lock_(false), top_level_(true),
nit: Reformat with one initializer per line if they don't all fit on a single
line.

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode59
src/v8threads.cc:59: 
nit: Two blank lines.

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode85
src/v8threads.cc:85: V8::Initialize();
I really don't like that we're doing a global initialization after an isolate
has already been preinitialized (and not just allocated). Can you change
initialization order here so that it looks more like the checks in api.cc? It'd
be nice to minimize the special code for the default isolate as a side-effect.

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode191
src/v8threads.cc:191: if (!lazily_archived_thread_.Equals(ThreadId::Invalid()))
{
Accidental edit? IsValid() should work.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:13383: context = v8::Context::New();
Why don't you use Context::Scope here?

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc
File test/cctest/test-lockers.cc (right):

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:1: // Copyright 2007-2009 the V8 project authors.
All rights reserved.
2011

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:66: explicit KangarooThread(v8::Isolate* isolate,
nit: explicit not required.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:67: v8::Handle<v8::Context> context, int value)
nit: Weird indentation.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:68: : Thread(NULL, "KangarooThread1"),
Why 1? There doesn't seem to be a second one.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:78: context_->Enter();
Please use Context::Scope where possible.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:104: TEST(KangarooIsolates) {
Please describe what a test does for non-trivial tests, e.g. "Test migrating an
isolate between the main thread and an additional thread."

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:108: v8::Locker locker_(isolate);
nit: Why _ ?

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:114: CompileRun("function getValue() { return 30;
}");
It'd be useful for this script to have some state, e.g. the first time it runs,
it returns "hello from main thread", then --- "hello from kangaroo", etc.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:132: class JoinableThread : public i::Thread {
To make this a bit more readable make JoinableThread *not* inherit from Thread
and provide Start and Join methods using a hidden per thread semaphore. Use a
hidden inner class to implement Run. Later we could use it to implement Thread
on the platforms that reuse thread ids.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:144: void Run() {
nit: Keep "virtual" when overriding.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:214: context->Exit();
nit: Fix indentation.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:231: class IsolateLockingTwiceThread : public
JoinableThread {
LockingTwice -> NestedLocking

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:264: class LockIsolateAndCalculateFibThread : public
JoinableThread {
Isn't this the same as IsolateLockingThreadWithLocalContext?

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:284: explicit
SeparateIsolatesLocksNonexclusiveThreadA(v8::Isolate* isolate1,
nit: explicit not required.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:286: : JoinableThread(NULL,
"ASeparateIsolatesLocksNonexclusiveThreadA"),
Why "A"? There doesn't seem to be a "B" variant of this.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:359: context->Exit();
Do we have to exit the context here?

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:361: v8::Unlocker unlocker(isolate_);
Hmm, do we really want to be able to unlock some arbitrary isolate? Maybe the
unlocker should support only the currently entered isolate and do Exit/Enter for
us?

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:370: CalcFibAndCheck();
context->Exit()?

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:417: }
context->Exit()?

[Dmitry Lomov, 2011-04-19 01:50:47]

New patch set: 
1) Addresses code review feedback
2) Moves PreInit back to under a lock, and moves Isolate::thread_manager_
initialization to isolate constructor. 

Thanks,
Mitya

http://codereview.chromium.org/6788023/diff/11001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3408
include/v8.h:3408: explicit Locker(Isolate* isolate);
On 2011/04/13 00:54:12, dslomov wrote:
> Yes, this sounds reasonable to me
> On 2011/04/13 00:45:10, Dmitry Titov wrote:
> > Consider: If it was:
> > explicit Locker(Isolate* isolate = NULL);
> > then there would be only 1 ctor, 1 IsLocked and no Init() method in this
> > API-specifying file... Same for Unlocker.
> > See TerminateExecution() in this file for precedent.
> 

Done.

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3432
include/v8.h:3432: * Returns whether or not the locker for a given isolate is
locked by the current thread.
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Long line.

Done.

http://codereview.chromium.org/6788023/diff/11001/include/v8.h#newcode3434
include/v8.h:3434: static bool IsLocked(Isolate * isolate);
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Drop space before '*'.

Done.

http://codereview.chromium.org/6788023/diff/11001/src/execution.cc
File src/execution.cc (right):

http://codereview.chromium.org/6788023/diff/11001/src/execution.cc#newcode461
src/execution.cc:461: isolate_->FindOrAllocatePerThreadDataForThisThread();
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Are you sure you need "OrAllocate" here?

It is actually the if below that was bogus - done

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc
File src/isolate.cc (left):

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc#oldcode643
src/isolate.cc:643: ASSERT(Isolate::Current() == this);
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Removing this is dangerous (at least for now), because passing and caching the
> isolate pointer is an optimization and some code may still use
> Isolate::Current() with hard to predict effects. Can you just set it in the
> thread local slot like it's done in the de-initialization code? I know it's
> ugly, but it's safer.

Done.

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc#newcode312
src/isolate.cc:312: 
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Two blank lines between functions.

Done.

http://codereview.chromium.org/6788023/diff/11001/src/isolate.cc#newcode323
src/isolate.cc:323: void Isolate::EnsurePreinitializedForThisThread() {
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> ForThisThread doesn't make much sense, since the isolate can only be
> preinitialized once. How about LockedPreInit?

I just removed this.

http://codereview.chromium.org/6788023/diff/11001/src/stub-cache.cc
File src/stub-cache.cc (left):

http://codereview.chromium.org/6788023/diff/11001/src/stub-cache.cc#oldcode45
src/stub-cache.cc:45: ASSERT(isolate == Isolate::Current());
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Please restore this (see my comment in isolate.cc).

Done.

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc
File src/v8threads.cc (right):

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode48
src/v8threads.cc:48: : has_lock_(false), top_level_(true),
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Reformat with one initializer per line if they don't all fit on a single
> line.

Done.

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode59
src/v8threads.cc:59: 
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Two blank lines.

Done.

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode85
src/v8threads.cc:85: V8::Initialize();
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> I really don't like that we're doing a global initialization after an isolate
> has already been preinitialized (and not just allocated). Can you change
> initialization order here so that it looks more like the checks in api.cc?
It'd
> be nice to minimize the special code for the default isolate as a side-effect.

Done.

http://codereview.chromium.org/6788023/diff/11001/src/v8threads.cc#newcode191
src/v8threads.cc:191: if (!lazily_archived_thread_.Equals(ThreadId::Invalid()))
{
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Accidental edit? IsValid() should work.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-api.cc#new...
test/cctest/test-api.cc:13383: context = v8::Context::New();
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Why don't you use Context::Scope here?

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc
File test/cctest/test-lockers.cc (right):

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:1: // Copyright 2007-2009 the V8 project authors.
All rights reserved.
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> 2011

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:66: explicit KangarooThread(v8::Isolate* isolate,
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: explicit not required.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:67: v8::Handle<v8::Context> context, int value)
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Weird indentation.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:68: : Thread(NULL, "KangarooThread1"),
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Why 1? There doesn't seem to be a second one.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:78: context_->Enter();
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Please use Context::Scope where possible.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:108: v8::Locker locker_(isolate);
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Why _ ?

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:114: CompileRun("function getValue() { return 30;
}");
It has a state of sorts - note that KangarooThread evaluates a different script
- this one defines a getValue, and KanagrooThread verifies that this function is
defined and returns what it should.
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> It'd be useful for this script to have some state, e.g. the first time it
runs,
> it returns "hello from main thread", then --- "hello from kangaroo", etc.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:132: class JoinableThread : public i::Thread {
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> To make this a bit more readable make JoinableThread *not* inherit from Thread
> and provide Start and Join methods using a hidden per thread semaphore. Use a
> hidden inner class to implement Run. Later we could use it to implement Thread
> on the platforms that reuse thread ids.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:144: void Run() {
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Keep "virtual" when overriding.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:214: context->Exit();
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: Fix indentation.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:231: class IsolateLockingTwiceThread : public
JoinableThread {
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> LockingTwice -> NestedLocking

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:264: class LockIsolateAndCalculateFibThread : public
JoinableThread {
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Isn't this the same as IsolateLockingThreadWithLocalContext?

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:284: explicit
SeparateIsolatesLocksNonexclusiveThreadA(v8::Isolate* isolate1,
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> nit: explicit not required.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:286: : JoinableThread(NULL,
"ASeparateIsolatesLocksNonexclusiveThreadA"),
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Why "A"? There doesn't seem to be a "B" variant of this.

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:359: context->Exit();
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Do we have to exit the context here?
Yes because we exit the isolate - in the next line.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:361: v8::Unlocker unlocker(isolate_);
Here is what I think on the subject:
1. I do not have a good real-life use case for Unlocker, so I have to argue from
aesthetics, which is not very convincing
2. Keeping Lock/Unlock and Enter/Exit orthogonal has a nice orthogonality to it
:)
3. Having to exit manually, as in the above code, is a bit on the ugly side;
however Unlocker that automatically exists suffers from two issues:
a) Unlocker unlocks all the Lockers that it is under, but it cannot possibly
exit all the scopes, as in:
  Locker lock(i1);
  i1->Enter();   
  Locker lock2(i1);
  i2->Enter();
  i1->Enter(); 
  Unlocker unlock(I1)
Unlocker can unlock all the locks, but it cannot exit i1 correctly in this case;
and if unlocker automatically exits, the user may be under the impression that
it can.
b) if Unlocker exits, it is only natural that Locker enters. Shall we do that as
well?

On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> Hmm, do we really want to be able to unlock some arbitrary isolate? Maybe the
> unlocker should support only the currently entered isolate and do Exit/Enter
for
> us?

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:370: CalcFibAndCheck();
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> context->Exit()?

Done.

http://codereview.chromium.org/6788023/diff/11001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:417: }
On 2011/04/15 00:29:39, Vitaly Repeshko wrote:
> context->Exit()?

Done.

[Vitaly Repeshko, 2011-04-21 21:15:27]

[+ Mads]

LGTM with comments.

I convinced myself we should keep locking/unlocking and entering/exiting an
isolate separate. So this Unlocker implementation and usage is fine. But please
format it like I proposed in the API, i.e. put both Exit and Enter outside of
the Unlocker scope.

As for closing other scopes, I don't think anything will break if we don't
explicitly exit a context before exiting an isolate we plan to re-enter. So for
consistency with handle scopes, try dropping unnecessary context Exit/Enter
pairs.


Thanks,
Vitaly

http://codereview.chromium.org/6788023/diff/21001/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3324
include/v8.h:3324: * If the isolate is only accessed on a single thread, it can
be
I think this paragraph adds little value and should be removed.

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3329
include/v8.h:3329: * If the isolate is accessed from multiple threads of a
process,
"the isolate" -> "an isolate"
"of a process" -> ", even when accesses are synchronized by external means"

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3330
include/v8.h:3330: * its use MUST be protected with appropriate v8::Locker use.
Replace the line with "v8::Locker must be used."

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3331
include/v8.h:3331: * You can do this by constructing a v8::Locker object to
guard
Replace the paragraph with "v8::Locker is a scoped lock object. While it's
active (i.e. between its construction and destruction) the current thread is
allowed to use the locked isolate. V8 guarantees that an isolate can be locked
by at most one thread at any time. In other words, the scope of a v8::Locker is
a critical section.

Sample usage:"

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3341
include/v8.h:3341: *   v8::Locker locker(isolate);
This snippet should show using Isolate::Scope.

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3353
include/v8.h:3353: * {
There should be isolate->Exit() before '{' and isolate->Enter() after '}'.

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3377
include/v8.h:3377: * {
This snippet should use isolate as well.

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3393
include/v8.h:3393: */
We need a special section describing the legacy usage with no explicit
entering/exiting of the default isolate.

http://codereview.chromium.org/6788023/diff/21001/include/v8.h#newcode3429
include/v8.h:3429: * Returns whether or not the locker for a given isolate, or
default isolate if NULL is given,
nit: Line too long.

http://codereview.chromium.org/6788023/diff/21001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6788023/diff/21001/src/heap.h#newcode1941
src/heap.h:1941: explicit DisallowAllocationFailure()  {
nit: "explicit" not required and remove extra space before '{'.

http://codereview.chromium.org/6788023/diff/21001/src/isolate.cc
File src/isolate.cc (right):

http://codereview.chromium.org/6788023/diff/21001/src/isolate.cc#newcode472
src/isolate.cc:472: thread_manager_ = new ThreadManager();
Add a note that thread manager is initialized early to support locking an
isolate before it's entered.

http://codereview.chromium.org/6788023/diff/21001/src/isolate.cc#newcode676
src/isolate.cc:676: DisallowAllocationFailure disallow_allocation_failure();
nit: Drop ().

http://codereview.chromium.org/6788023/diff/21001/src/isolate.cc#newcode758
src/isolate.cc:758: DisallowAllocationFailure disallow_allocation_failure();
nit: Drop ().

http://codereview.chromium.org/6788023/diff/21001/src/isolate.cc#newcode763
src/isolate.cc:763: 
nit: Remove extra blank line.

http://codereview.chromium.org/6788023/diff/21001/src/isolate.h
File src/isolate.h (right):

http://codereview.chromium.org/6788023/diff/21001/src/isolate.h#newcode1077
src/isolate.h:1077: // Preinitialize this isolate for current thread.
Thread-safe.
Unused? Remove.

http://codereview.chromium.org/6788023/diff/21001/src/v8threads.cc
File src/v8threads.cc (right):

http://codereview.chromium.org/6788023/diff/21001/src/v8threads.cc#newcode92
src/v8threads.cc:92: return internal_isolate->thread_manager()->
nit: Fits on a single line?

http://codereview.chromium.org/6788023/diff/21001/src/v8threads.cc#newcode112
src/v8threads.cc:112: if (isolate_ == NULL)
nit: Use {}.

http://codereview.chromium.org/6788023/diff/21001/src/v8threads.cc#newcode121
src/v8threads.cc:121: 
nit: Remove extra blank line.

http://codereview.chromium.org/6788023/diff/21001/src/v8threads.cc#newcode142
src/v8threads.cc:142: 
Same nit.

http://codereview.chromium.org/6788023/diff/21001/test/cctest/test-lockers.cc
File test/cctest/test-lockers.cc (right):

http://codereview.chromium.org/6788023/diff/21001/test/cctest/test-lockers.cc...
test/cctest/test-lockers.cc:29: #include <vector>
Remove unnecessary includes.

[Dmitry Lomov, 2011-04-30 02:50:28]

This additional patch fixes Locker and Unlocker properly enter and exit default
isolate. This fixes TerminateMultipleV8Threads test. 

The key problem with that test was that the Locker entered a default isolate
before thread_manager->RestoreThread call, and that interferes with lazy thread
archiving.

[Vitaly Repeshko, 2011-05-03 16:41:18]

LGTM

http://codereview.chromium.org/6788023/diff/24003/src/v8threads.cc
File src/v8threads.cc (right):

http://codereview.chromium.org/6788023/diff/24003/src/v8threads.cc#newcode132
src/v8threads.cc:132: isolate_->Enter();
Weird indent.

[Dmitry Lomov, 2011-05-05 18:37:33]

Additional fixes for arm/mips. Pushing an isolate parameter through to the
Simulator.

(I have uploaded the whole patch, including all previous changes - let me know
if it is easy enough to reviw, or I can prep a smaller patch with just Simulator
changes)

Thanks!
Mitya

[Vitaly Repeshko, 2011-05-05 18:41:29]

LGTM