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.
http://codereview.chromium.org/10640012/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
