Add a second kind of HandleScope that ties the lifetime of Handles created in its scope to the life…

Add a second kind of HandleScope that ties the lifetime of Handles created in its scope to the lifetime of a given CompilationInfo.


BUG=
TEST=

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

[Erik Corry, 2012-06-22 09:39:56]

I'm not happy with having references to compilation in the api.h since this
stuff is not necessarily tied to compilation only.  Also, this proposal only
allows one saved scope per VM so you can never have more than one thread
compiling.

Can we instead have register/deregister functions that allow more than one scope
to be saved at a time?

[sanjoy, 2012-06-22 10:04:17]

> I'm not happy with having references to compilation in the api.h since this
> stuff is not necessarily tied to compilation only.  Also, this proposal only

Are you suggesting renaming things like last_block_before_compilation_
to something more generic?

> allows one saved scope per VM so you can never have more than one thread
> compiling.
> 
> Can we instead have register/deregister functions that allow more than one
scope
> to be saved at a time?

I think this approach does allow multiple threads of compilation.  The
GatheringHandleScope needs to be constructed and destructed on the
mutator / GC thread synchronously (since we create Handles then), but
other than that, any number of compilation threads could be running at
the same time.  The fix for allowing correct root traversal (using
last_block_before_compilation_) is only needed when there is a GC
_while_ we're creating Handles inside a GatheringHandleScope (which
doesn't happens on the mutator / GC thread).

[sanjoy, 2012-06-22 10:05:22]

> _while_ we're creating Handles inside a GatheringHandleScope (which
> doesn't happens on the mutator / GC thread).
I meant "which happens on the mutator / GC thread".

[Erik Corry, 2012-06-22 11:08:36]

I am worried that we could accidentally dereference the handles from the other
thread and it would work most of the time and occasionally crash.  I would
suggest that in debug mode we could 'pickle' the object pointers in the arrays
by flipping a bit.  Then they will cause a crash if they are accessed.  On GC
they need to be unpickled and repickled and of course when we are back on the
main thread we need to unpickle them.

http://codereview.chromium.org/10640012/diff/2001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6376
src/api.cc:6376: template<bool HasCompilationBlock>
This should be named_like_this, but I wonder whether this templatization is
worth it.  Do you have benchmark runs showing that just testing
last_block_before_compilation_ is too slow?

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6453
src/api.cc:6453: // ASSERT this.
ASSERT -> Assert
(in the comment)

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6476
src/api.cc:6476: if (extension->next) {
We don't allow implicit conversions to bool.

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6479
src/api.cc:6479: if (extension->previous) {
and here

http://codereview.chromium.org/10640012/diff/2001/src/api.h
File src/api.h (right):

http://codereview.chromium.org/10640012/diff/2001/src/api.h#newcode415
src/api.h:415: hidden_extensions_head_(NULL) { }
Instead of 'hidden' may I suggest 'persistent'.  That is what we have used other
places where we have non-stack based handles that do not have a scope based
lifetime.

http://codereview.chromium.org/10640012/diff/2001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10640012/diff/2001/src/compiler.h#newcode178
src/compiler.h:178: void RememberHiddenExtensions(
Could be called StorePersistentExtensions.

[Erik Corry, 2012-06-22 11:09:21]

On 2012/06/22 10:04:17, sanjoy wrote:
> I think this approach does allow multiple threads of compilation.  The
> GatheringHandleScope needs to be constructed and destructed on the
> mutator / GC thread synchronously (since we create Handles then), but
> other than that, any number of compilation threads could be running at
> the same time.  The fix for allowing correct root traversal (using
> last_block_before_compilation_) is only needed when there is a GC
> _while_ we're creating Handles inside a GatheringHandleScope (which
> doesn't happens on the mutator / GC thread).

Ah yes, now I see this part.

[sanjoy, 2012-06-22 11:26:59]

> I am worried that we could accidentally dereference the handles from the other
> thread and it would work most of the time and occasionally crash.  I would
> suggest that in debug mode we could 'pickle' the object pointers in the arrays
> by flipping a bit.  Then they will cause a crash if they are accessed.  On GC
> they need to be unpickled and repickled and of course when we are back on the
> main thread we need to unpickle them.

The plan is to first _allow_ dereferencing Handles from the compiler
thread (and lock out the phases of the GC that move stuff).  I'm
trying to get an estimate on how much work it will be to not deref
Handles in crankshaft altogether, and if that seems doable what you
said can be implemented.  Alternatively we could, in debug mode, have
Handle::operator* check a TLS variable and assert the deref doesn't
happen in the compiler thread.

> This should be named_like_this, but I wonder whether this templatization is
> worth it.  Do you have benchmark runs showing that just testing
> last_block_before_compilation_ is too slow?

No, but this looked like a small, local change.  :) I'll revert it,
and add it back if this seems to slow things down.

[Erik Corry, 2012-06-22 11:37:51]

On 2012/06/22 11:26:59, sanjoy wrote:
> The plan is to first _allow_ dereferencing Handles from the compiler
> thread (and lock out the phases of the GC that move stuff).  I'm

Risky plan, because what can you do with the object once you have got it by
dereferencing the handle?  It's being mutated by another thread that does no
locking.  You can test it for Smi-ness, but most other operations (look at the
map, for example) are not safe.

> trying to get an estimate on how much work it will be to not deref
> Handles in crankshaft altogether, and if that seems doable what you
> said can be implemented.  Alternatively we could, in debug mode, have
> Handle::operator* check a TLS variable and assert the deref doesn't
> happen in the compiler thread.
> 
> > This should be named_like_this, but I wonder whether this templatization is
> > worth it.  Do you have benchmark runs showing that just testing
> > last_block_before_compilation_ is too slow?
> 
> No, but this looked like a small, local change.  :) I'll revert it,
> and add it back if this seems to slow things down.

[sanjoy, 2012-06-22 11:44:36]

> Risky plan, because what can you do with the object once you have got it by
> dereferencing the handle?  It's being mutated by another thread that does no
> locking.  You can test it for Smi-ness, but most other operations (look at
> the
> map, for example) are not safe.

Right.  The GC isn't the only thing that mutates the HeapObject.

This is a problem because even if I manage gathering all the
information before I move the compilation to the second thread, how do
I know that the information is still valid when I compile (or _while_
I compile)?  I think the right solution is to defer all those actions
to after the compilation has been done on the compiler thread.  It
will be quite tricky to do that AND do a significant amount of work in
the compiler thread.

[sanjoy, 2012-06-22 20:08:10]

http://codereview.chromium.org/10640012/diff/2001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6376
src/api.cc:6376: template<bool HasCompilationBlock>
On 2012/06/22 11:08:36, Erik Corry wrote:
> This should be named_like_this, but I wonder whether this templatization is
> worth it.  Do you have benchmark runs showing that just testing
> last_block_before_compilation_ is too slow?

Reverted.

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6453
src/api.cc:6453: // ASSERT this.
On 2012/06/22 11:08:36, Erik Corry wrote:
> ASSERT -> Assert
> (in the comment)

Fixed.

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6476
src/api.cc:6476: if (extension->next) {
On 2012/06/22 11:08:36, Erik Corry wrote:
> We don't allow implicit conversions to bool.

Fixed.

http://codereview.chromium.org/10640012/diff/2001/src/api.cc#newcode6479
src/api.cc:6479: if (extension->previous) {
On 2012/06/22 11:08:36, Erik Corry wrote:
> and here

Fixed.

http://codereview.chromium.org/10640012/diff/2001/src/api.h
File src/api.h (right):

http://codereview.chromium.org/10640012/diff/2001/src/api.h#newcode415
src/api.h:415: hidden_extensions_head_(NULL) { }
On 2012/06/22 11:08:36, Erik Corry wrote:
> Instead of 'hidden' may I suggest 'persistent'.  That is what we have used
other
> places where we have non-stack based handles that do not have a scope based
> lifetime.

Done.

http://codereview.chromium.org/10640012/diff/2001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10640012/diff/2001/src/compiler.h#newcode178
src/compiler.h:178: void RememberHiddenExtensions(
On 2012/06/22 11:08:36, Erik Corry wrote:
> Could be called StorePersistentExtensions.

Fixed.

[danno, 2012-06-24 11:07:03]

Sorry for the long DBC, but I some substantial feedback. I think the plumbing
here is bit complicated and ownership/lifecycle responsibility is muddy. I would
suggest more something more like this:

in HGraph:

CompilationHandleScope scope; // Constructor calls
impl->BeginDeferredReleaseHandleScope(), remembers result in deferred_scope_

....
// Transfer ownership of DeferredReleaseHandleScope to CompilationInfo.
DeferredReleaseHandleScope* scope = scope->DetachHandles();  // Calls
scope.deferred_scope_->Defer(), returns deferred_scope_.
compilation_info->set_deferred_release_handle_scope(scope);

// in ~CompilationInfo:
delete deferred_release_handle_scope_;  // (or better yet, managed by a RAII
wrapper)

// DeferredReleaseHandleScope's descritor is responsible for cleaning itself
up/removing it from the ScopeImplementer's list

http://codereview.chromium.org/10640012/diff/10/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/10640012/diff/10/src/api.cc#newcode6393
src/api.cc:6393: ASSERT(!has_compilation_block_ ||
I can't find has_compilation_block_ anywhere else.

http://codereview.chromium.org/10640012/diff/10/src/api.h
File src/api.h (right):

http://codereview.chromium.org/10640012/diff/10/src/api.h#newcode431
src/api.h:431: struct PersistentExtensions {
Perhaps DeferredReleaseHandleScope?

http://codereview.chromium.org/10640012/diff/10/src/api.h#newcode443
src/api.h:443: PersistentExtensions* PersistExtensions(internal::Object**
prev_limit);
BeginDeferredHandleScope?

http://codereview.chromium.org/10640012/diff/10/src/api.h#newcode444
src/api.h:444: void ReleaseExtensions(PersistentExtensions* extension);
Should be a private method only called by the DeferredReleaseHandleScopes'
destructor.

http://codereview.chromium.org/10640012/diff/10/src/api.h#newcode463
src/api.h:463: void begin_persistence() {
I think you can merge this with PersistExtensions if it becomes
BeginDeferredHandleScope.

http://codereview.chromium.org/10640012/diff/10/src/api.h#newcode470
src/api.h:470: void end_persistence() {
This should by a private method called "DeferHandleScope" that is only called by
the "Defer" method on the DeferredHandleScope, which itself should only be
called by the CompilationHandleScopes's DetachHandles method.

http://codereview.chromium.org/10640012/diff/10/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10640012/diff/10/src/compiler.cc#newcode111
src/compiler.cc:111: impl->ReleaseExtensions(persistent_extensions_);
should be "delete persistent_extensions_" and the descriptor of the
DeferredReleaseScopeHandle should to the rest.

http://codereview.chromium.org/10640012/diff/10/src/handles.cc
File src/handles.cc (right):

http://codereview.chromium.org/10640012/diff/10/src/handles.cc#newcode964
src/handles.cc:964: Isolate* isolate = info->isolate();
You should be able to put the code below in the ScopeHandleImplementation if you
here call:

impl->BeginDeferredHandleScope() 

here and remember the deferred handle scope. BTW, BeginDeferredHandleScope
should take a HandleScope as parameter rather than the explicit prev_limit_
parameter to encapsulate the implementation.

http://codereview.chromium.org/10640012/diff/10/src/handles.cc#newcode986
src/handles.cc:986: impl->PersistExtensions(prev_limit_);
All of the code below can be pushed to the impl if you call the DeferHandleScope
method here on the result of the BeginDeferredHandleScope method that you called
in the constructor.

http://codereview.chromium.org/10640012/diff/10/src/handles.h
File src/handles.h (right):

http://codereview.chromium.org/10640012/diff/10/src/handles.h#newcode175
src/handles.h:175: class GatheringHandleScope {
Why not just call this CompilationHandleScope?

http://codereview.chromium.org/10640012/diff/10/src/handles.h#newcode175
src/handles.h:175: class GatheringHandleScope {
This guy should be responsible for creating the "PersistentExtension", not the
CompilationInfo itself. It should hand off the PersistentExtension to the
CompilationInfo in a "detach" operation.

[sanjoy, 2012-06-25 14:09:31]

In this patch, the HandleScopeImplementer creates a
DeferredHandleScope, which has a 'Detach()' method that returns a
DeferredHandles object.  The Handles created after the creation of the
DeferredHandleScope have their lifetime tied to the lifetime of the
DeferredHandles object -- they die when the DeferredHandles object is
destroyed.

Client code looks like this (CompilationHandleScope, for instance):

{
// Remember the next and limit pointers in the currently active
// HandleScopeData using a regular HandleScope.
HandleScope scope;

DeferredHandleScope* dhs =
handle_scope_implementer->CreateDeferredHandleScope();

// Create a bunch of handles

DeferredHandles* dh = dhs->Detach();
delete dhs; // Not needed anymore.

// Old limit and next restored here.
}

// ...
// Once we no longer need the Handles we created ...

delete dh;

[Erik Corry, 2012-06-27 10:28:15]

LGTM

http://codereview.chromium.org/10640012/diff/27001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/10640012/diff/27001/src/api.cc#newcode6409
src/api.cc:6409: for (DeferredHandles* deferred = deferred_handles_head_;
deferred;
Please put the parts of a for loop either all on one line or on 3 lines, not 2
and 1.  Also we don't allow implicit boolean conversions so you have to compare
with NULL.

http://codereview.chromium.org/10640012/diff/27001/src/api.cc#newcode6450
src/api.cc:6450: ASSERT(last_handle_before_deferred_block_);
No implicit boolean conversions.

http://codereview.chromium.org/10640012/diff/27001/src/handles.cc
File src/handles.cc (right):

http://codereview.chromium.org/10640012/diff/27001/src/handles.cc#newcode980
src/handles.cc:980: 
Missing newline.

http://codereview.chromium.org/10640012/diff/27001/src/handles.cc#newcode1001
src/handles.cc:1001:
CompilationHandleScope::CompilationHandleScope(CompilationInfo* info)
This should also go somewhere else (perhaps inlined in the class declaration).

http://codereview.chromium.org/10640012/diff/27001/src/handles.h
File src/handles.h (right):

http://codereview.chromium.org/10640012/diff/27001/src/handles.h#newcode176
src/handles.h:176: // alive till as long as the DeferredHandles object is alive.
till as long as -> as long as

http://codereview.chromium.org/10640012/diff/27001/src/handles.h#newcode194
src/handles.h:194: class CompilationInfo;
This declaration and the CompilationHandleScope are uses of handles and not part
of the handles interface, so they should go somewhere else, like perhaps
hydrogen.h

http://codereview.chromium.org/10640012/diff/27001/src/handles.h#newcode197
src/handles.h:197: // A wrapper around a CompilationInfo that Detaches the
Handles from
Detaches -> detaches

[sanjoy, 2012-06-27 11:17:09]

Will land after running tests in debug mode.

http://codereview.chromium.org/10640012/diff/27001/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/10640012/diff/27001/src/api.cc#newcode6409
src/api.cc:6409: for (DeferredHandles* deferred = deferred_handles_head_;
deferred;
On 2012/06/27 10:28:15, Erik Corry wrote:
> Please put the parts of a for loop either all on one line or on 3 lines, not 2
> and 1.  Also we don't allow implicit boolean conversions so you have to
compare
> with NULL.

Fixed.

http://codereview.chromium.org/10640012/diff/27001/src/api.cc#newcode6450
src/api.cc:6450: ASSERT(last_handle_before_deferred_block_);
On 2012/06/27 10:28:15, Erik Corry wrote:
> No implicit boolean conversions.

Fixed.

http://codereview.chromium.org/10640012/diff/27001/src/handles.cc
File src/handles.cc (right):

http://codereview.chromium.org/10640012/diff/27001/src/handles.cc#newcode980
src/handles.cc:980: 
On 2012/06/27 10:28:15, Erik Corry wrote:
> Missing newline.

Fixed.

http://codereview.chromium.org/10640012/diff/27001/src/handles.cc#newcode1001
src/handles.cc:1001:
CompilationHandleScope::CompilationHandleScope(CompilationInfo* info)
On 2012/06/27 10:28:15, Erik Corry wrote:
> This should also go somewhere else (perhaps inlined in the class declaration).

Moved to compiler.h

http://codereview.chromium.org/10640012/diff/27001/src/handles.h
File src/handles.h (right):

http://codereview.chromium.org/10640012/diff/27001/src/handles.h#newcode176
src/handles.h:176: // alive till as long as the DeferredHandles object is alive.
On 2012/06/27 10:28:15, Erik Corry wrote:
> till as long as -> as long as

Fixed.

http://codereview.chromium.org/10640012/diff/27001/src/handles.h#newcode194
src/handles.h:194: class CompilationInfo;
On 2012/06/27 10:28:15, Erik Corry wrote:
> This declaration and the CompilationHandleScope are uses of handles and not
part
> of the handles interface, so they should go somewhere else, like perhaps
> hydrogen.h

Moved to compiler.h.

http://codereview.chromium.org/10640012/diff/27001/src/handles.h#newcode197
src/handles.h:197: // A wrapper around a CompilationInfo that Detaches the
Handles from
On 2012/06/27 10:28:15, Erik Corry wrote:
> Detaches -> detaches

Fixed.