Make assertion scopes thread safe.

Make assertion scopes thread safe.

R=svenpanne@chromium.org
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=14919

[Sven Panne, 2013-06-03 09:58:23]

https://codereview.chromium.org/15691017/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15691017/diff/1/src/api.cc#newcode6274
src/api.cc:6274: v8::AssertNoGCScope::AssertNoGCScope(v8::Isolate* isolate)
This class is identical to DisallowHeapAllocation (modulo namespace etc.), and
SetIsAllowed is ugly. There are several ways to fix this:

   * Duplicate what the DisallowHeapAllocation template does here.

   * Remove BASE_EMBEDDED from PerThreadAssertScope and use an (opaque) pointer
in AssertNoGCScope + new/delete.

Both options are not really nice, and I don't really have a strong preference
(perhaps the first option?). Nevertheless, I think that both approaches are
nicer than keeping SetIsAllowed.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h
File src/assert-scope.h (right):

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode61
src/assert-scope.h:61: bool get(PerThreadAssertType type) {
Missing const.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode66
src/assert-scope.h:66: static Thread::LocalStorageKey thread_local_key;
This is in the wrong class: It has nothing to do with the data itself, so move
it to PerThreadAssertScope.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode95
src/assert-scope.h:95: static void SetIsAllowed(bool is_allowed) {
This is a real wart, because it totally breaks the fundamental contract of this
class. We should remove it, see comment elsewhere.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode146
src/assert-scope.h:146: #ifdef DEBUG
I don't think we need this #ifdef nor the macro below: Just add a const char*
default argument to PerThreadAssertScope's constructor and use
AllowDeferredHandleDereference.

https://codereview.chromium.org/15691017/diff/1/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/15691017/diff/1/src/handles.h#newcode336
src/handles.h:336: class RequireNewHandleScope BASE_EMBEDDED {
As discussed offline, rename this to "SealHandleScope".

https://codereview.chromium.org/15691017/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/15691017/diff/1/src/isolate.cc#newcode399
src/isolate.cc:399: PerThreadAssertData::thread_local_key =
Thread::CreateThreadLocalKey();
Hmmm, I think all this key creation code should live in
V8::InitializeOncePerProcessImpl, but perhaps this cleanup belongs into a
different CL.

[Yang, 2013-06-03 13:37:15]

Please take another look.

https://codereview.chromium.org/15691017/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15691017/diff/1/src/api.cc#newcode6274
src/api.cc:6274: v8::AssertNoGCScope::AssertNoGCScope(v8::Isolate* isolate)
On 2013/06/03 09:58:23, Sven Panne wrote:
> This class is identical to DisallowHeapAllocation (modulo namespace etc.), and
> SetIsAllowed is ugly. There are several ways to fix this:
> 
>    * Duplicate what the DisallowHeapAllocation template does here.
> 
>    * Remove BASE_EMBEDDED from PerThreadAssertScope and use an (opaque)
pointer
> in AssertNoGCScope + new/delete.
> 
> Both options are not really nice, and I don't really have a strong preference
> (perhaps the first option?). Nevertheless, I think that both approaches are
> nicer than keeping SetIsAllowed.

Done.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h
File src/assert-scope.h (right):

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode61
src/assert-scope.h:61: bool get(PerThreadAssertType type) {
On 2013/06/03 09:58:23, Sven Panne wrote:
> Missing const.

Done.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode66
src/assert-scope.h:66: static Thread::LocalStorageKey thread_local_key;
On 2013/06/03 09:58:23, Sven Panne wrote:
> This is in the wrong class: It has nothing to do with the data itself, so move
> it to PerThreadAssertScope.

Done.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode95
src/assert-scope.h:95: static void SetIsAllowed(bool is_allowed) {
On 2013/06/03 09:58:23, Sven Panne wrote:
> This is a real wart, because it totally breaks the fundamental contract of
this
> class. We should remove it, see comment elsewhere.

We use this in the deoptimizer and heap as well (for example in GC prologue and
GC epilogue).

I changed those places using new/delete. If you don't like the way it is, we can
think of something else.

https://codereview.chromium.org/15691017/diff/1/src/assert-scope.h#newcode146
src/assert-scope.h:146: #ifdef DEBUG
On 2013/06/03 09:58:23, Sven Panne wrote:
> I don't think we need this #ifdef nor the macro below: Just add a const char*
> default argument to PerThreadAssertScope's constructor and use
> AllowDeferredHandleDereference.

I'll just use the variable name to state its purpose.

https://codereview.chromium.org/15691017/diff/1/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/15691017/diff/1/src/handles.h#newcode336
src/handles.h:336: class RequireNewHandleScope BASE_EMBEDDED {
On 2013/06/03 09:58:23, Sven Panne wrote:
> As discussed offline, rename this to "SealHandleScope".

Done.

https://codereview.chromium.org/15691017/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/15691017/diff/1/src/isolate.cc#newcode399
src/isolate.cc:399: PerThreadAssertData::thread_local_key =
Thread::CreateThreadLocalKey();
On 2013/06/03 09:58:23, Sven Panne wrote:
> Hmmm, I think all this key creation code should live in
> V8::InitializeOncePerProcessImpl, but perhaps this cleanup belongs into a
> different CL.

Will do in a follow-up CL.

[Sven Panne, 2013-06-03 14:11:46]

LGTM with a nit.

https://codereview.chromium.org/15691017/diff/11001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/15691017/diff/11001/src/api.cc#newcode6280
src/api.cc:6280:
reinterpret_cast<i::DisallowHeapAllocation*>(disallow_heap_allocation_);
I think this has to be a static_cast, IIRC.

[Yang, 2013-06-03 15:33:08]

Committed patchset #3 manually as r14919 (presubmit successful).