"Deiceolate" Thread classes.

"Deiceolate" Thread classes.

Thread class was receiving an isolate parameter by default.
This approact violates the assumption that only VM threads
can have an associated isolate, and can lead to troubles,
because accessing the same isolate from different threads
leads to race conditions.

This was found by investigating mysterious failures of the
CPU profiler layout test on Linux Chromium. As almost all
threads were associated with some isolate, the sampler was
trying to sample them.

As a side effect, we have also fixed the DebuggerAgent test.

Thanks to Vitaly for help in fixing isolates handling!

R=vitalyr@chromium.org
BUG=none
TEST=none

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

[Vitaly Repeshko, 2011-06-10 08:18:50]

"Deisolate" maybe?

LGTM!

http://codereview.chromium.org/7003108/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/7003108/diff/1/include/v8.h#newcode3095
include/v8.h:3095: * Is V8 terminating JavaScript execution.
Please update the comment.

http://codereview.chromium.org/7003108/diff/1/src/debug.cc
File src/debug.cc (left):

http://codereview.chromium.org/7003108/diff/1/src/debug.cc#oldcode799
src/debug.cc:799: ASSERT(Isolate::Current() == isolate_);
Some of these asserts are useful as the isolate is not threaded through all the
calls here. The parts of the interface that are required to work without the
isolate being current should be clearly marked. Otherwise, we risk e.g. mixing
isolate heaps.

http://codereview.chromium.org/7003108/diff/1/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/debug.cc#newcode2640
src/debug.cc:2640: isolate_->compilation_cache()->Disable();
As we discussed this is actually unsafe. ListenersChanged may be called from any
thread and touches the compilation cache. This means we need a compilation cache
mutex. Having the debugger mutex only doesn't seem to be enough. Please file a
bug.

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc
File src/platform-openbsd.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode399
src/platform-openbsd.cc:399: Thread::Thread(Isolate* isolate, const Options&
options)
Won't compile.

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode407
src/platform-openbsd.cc:407: Thread::Thread(Isolate* isolate, const char* name)
Ditto.

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode427
src/platform-openbsd.cc:427: Thread::SetThreadLocal(Isolate::isolate_key(),
thread->isolate());
Remove this line.

http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc
File src/platform-solaris.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc#newcode385
src/platform-solaris.cc:385: isolate_(isolate),
Won't compile.

http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc#newcode391
src/platform-solaris.cc:391: Thread::Thread(Isolate* isolate, const char* name)
Ditto.

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc
File src/platform-win32.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1479
src/platform-win32.cc:1479: Thread::SetThreadLocal(Isolate::isolate_key(),
thread->isolate());
Remove this line.

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1495
src/platform-win32.cc:1495: Thread::Thread(Isolate* isolate, const Options&
options)
Won't compile.

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1503
src/platform-win32.cc:1503: Thread::Thread(Isolate* isolate, const char* name)
Ditto.

[mnaganov (inactive), 2011-06-10 09:31:11]

Thanks!

On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> "Deisolate" maybe?
> 

This is a mixture of "Deicing" (http://en.wikipedia.org/wiki/Deicing) and
"Iceolate" (http://www.youtube.com/watch?v=bCqHTu4ekME) :)

> LGTM!
> 
> http://codereview.chromium.org/7003108/diff/1/include/v8.h
> File include/v8.h (right):
> 
> http://codereview.chromium.org/7003108/diff/1/include/v8.h#newcode3095
> include/v8.h:3095: * Is V8 terminating JavaScript execution.
> Please update the comment.
> 
> http://codereview.chromium.org/7003108/diff/1/src/debug.cc
> File src/debug.cc (left):
> 
> http://codereview.chromium.org/7003108/diff/1/src/debug.cc#oldcode799
> src/debug.cc:799: ASSERT(Isolate::Current() == isolate_);
> Some of these asserts are useful as the isolate is not threaded through all
the
> calls here. The parts of the interface that are required to work without the
> isolate being current should be clearly marked. Otherwise, we risk e.g. mixing
> isolate heaps.
> 
> http://codereview.chromium.org/7003108/diff/1/src/debug.cc
> File src/debug.cc (right):
> 
> http://codereview.chromium.org/7003108/diff/1/src/debug.cc#newcode2640
> src/debug.cc:2640: isolate_->compilation_cache()->Disable();
> As we discussed this is actually unsafe. ListenersChanged may be called from
any
> thread and touches the compilation cache. This means we need a compilation
cache
> mutex. Having the debugger mutex only doesn't seem to be enough. Please file a
> bug.
> 
> http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc
> File src/platform-openbsd.cc (right):
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode399
> src/platform-openbsd.cc:399: Thread::Thread(Isolate* isolate, const Options&
> options)
> Won't compile.
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode407
> src/platform-openbsd.cc:407: Thread::Thread(Isolate* isolate, const char*
name)
> Ditto.
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode427
> src/platform-openbsd.cc:427: Thread::SetThreadLocal(Isolate::isolate_key(),
> thread->isolate());
> Remove this line.
> 
> http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc
> File src/platform-solaris.cc (right):
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc#newcode385
> src/platform-solaris.cc:385: isolate_(isolate),
> Won't compile.
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc#newcode391
> src/platform-solaris.cc:391: Thread::Thread(Isolate* isolate, const char*
name)
> Ditto.
> 
> http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc
> File src/platform-win32.cc (right):
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1479
> src/platform-win32.cc:1479: Thread::SetThreadLocal(Isolate::isolate_key(),
> thread->isolate());
> Remove this line.
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1495
> src/platform-win32.cc:1495: Thread::Thread(Isolate* isolate, const Options&
> options)
> Won't compile.
> 
>
http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1503
> src/platform-win32.cc:1503: Thread::Thread(Isolate* isolate, const char* name)
> Ditto.

[mnaganov (inactive), 2011-06-10 09:31:32]

http://codereview.chromium.org/7003108/diff/1/include/v8.h
File include/v8.h (right):

http://codereview.chromium.org/7003108/diff/1/include/v8.h#newcode3095
include/v8.h:3095: * Is V8 terminating JavaScript execution.
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Please update the comment.

Done.

http://codereview.chromium.org/7003108/diff/1/src/debug.cc
File src/debug.cc (left):

http://codereview.chromium.org/7003108/diff/1/src/debug.cc#oldcode799
src/debug.cc:799: ASSERT(Isolate::Current() == isolate_);
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Some of these asserts are useful as the isolate is not threaded through all
the
> calls here. The parts of the interface that are required to work without the
> isolate being current should be clearly marked. Otherwise, we risk e.g. mixing
> isolate heaps.

http://code.google.com/p/v8/issues/detail?id=1449

http://codereview.chromium.org/7003108/diff/1/src/debug.cc
File src/debug.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/debug.cc#newcode2640
src/debug.cc:2640: isolate_->compilation_cache()->Disable();
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> As we discussed this is actually unsafe. ListenersChanged may be called from
any
> thread and touches the compilation cache. This means we need a compilation
cache
> mutex. Having the debugger mutex only doesn't seem to be enough. Please file a
> bug.

http://code.google.com/p/v8/issues/detail?id=1448

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc
File src/platform-openbsd.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode399
src/platform-openbsd.cc:399: Thread::Thread(Isolate* isolate, const Options&
options)
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Won't compile.

Thanks! Somehow, I have missed this file.

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode407
src/platform-openbsd.cc:407: Thread::Thread(Isolate* isolate, const char* name)
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Ditto.

Done.

http://codereview.chromium.org/7003108/diff/1/src/platform-openbsd.cc#newcode427
src/platform-openbsd.cc:427: Thread::SetThreadLocal(Isolate::isolate_key(),
thread->isolate());
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Remove this line.

Done.

http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc
File src/platform-solaris.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc#newcode385
src/platform-solaris.cc:385: isolate_(isolate),
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Won't compile.

Done.

http://codereview.chromium.org/7003108/diff/1/src/platform-solaris.cc#newcode391
src/platform-solaris.cc:391: Thread::Thread(Isolate* isolate, const char* name)
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Ditto.

Done.

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc
File src/platform-win32.cc (right):

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1479
src/platform-win32.cc:1479: Thread::SetThreadLocal(Isolate::isolate_key(),
thread->isolate());
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Remove this line.

Hmm... I've actually did this once. Something has got messed up.
Fixed.

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1495
src/platform-win32.cc:1495: Thread::Thread(Isolate* isolate, const Options&
options)
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Won't compile.

Done.

http://codereview.chromium.org/7003108/diff/1/src/platform-win32.cc#newcode1503
src/platform-win32.cc:1503: Thread::Thread(Isolate* isolate, const char* name)
On 2011/06/10 08:18:50, Vitaly Repeshko wrote:
> Ditto.

Done.