Introduce Runtime_GetAllScopesDetails to get all scopes at once for a frame.

Introduce Runtime_GetAllScopesDetails to get all scopes at once for a frame.

This will reduce heavy ScopeIterator instantiations.
Once incorporated into chromium, will give 30% speed boost.

BUG=chromium:340285
LOG=Y
R=ulan@chromium.org, Yang, rossberg, ulan

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

[aandrey, 2014-03-04 09:11:39]

ping?

[ulan, 2014-03-04 10:41:58]

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js#newco...
src/mirror-debugger.js:1941: function ScopeMirror(frame, function, index,
opt_details) {
Maybe just "details" instead of "opt_details". "opt" is confusing, it probably
means "optional", but it can also meant "optimized" in v8. :)

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12328
src/runtime.cc:12328: ScopeIterator it(isolate, frame, inlined_jsframe_index);
This code seems to work, but it is fragile because it mixes handlified code with
raw pointers. ScopeIterator can cause GC, so after this point any raw pointer to
v8 heap from above may become invalid.

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12330
src/runtime.cc:12330: MaybeObject* maybe_object =
MaterializeScopeDetails(isolate, &it);
MaterializeScopeDetails looks already handlified, it could just return a handle,
instead of maybe_objects. The you wouldn't need checks and conversions below.

[aandrey, 2014-03-04 11:04:19]

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js#newco...
src/mirror-debugger.js:1941: function ScopeMirror(frame, function, index,
opt_details) {
On 2014/03/04 10:41:59, ulan wrote:
> Maybe just "details" instead of "opt_details". "opt" is confusing, it probably
> means "optional", but it can also meant "optimized" in v8. :)

Yes, it means "optional", and we do use "opt_" prefix all over the place (20+
matches in this file).

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12328
src/runtime.cc:12328: ScopeIterator it(isolate, frame, inlined_jsframe_index);
On 2014/03/04 10:41:59, ulan wrote:
> This code seems to work, but it is fragile because it mixes handlified code
with
> raw pointers. ScopeIterator can cause GC, so after this point any raw pointer
to
> v8 heap from above may become invalid.

You mean the JavaScriptFrame* pointer?
I'm not an expert here, but this code, up to the point, is just a carbon copy of
the above Runtime_GetScopeDetails.

The JavaScriptFrame class is BASE_EMBEDDED. Should it care on GC being run?

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12330
src/runtime.cc:12330: MaybeObject* maybe_object =
MaterializeScopeDetails(isolate, &it);
On 2014/03/04 10:41:59, ulan wrote:
> MaterializeScopeDetails looks already handlified, it could just return a
handle,
> instead of maybe_objects. The you wouldn't need checks and conversions below.

As far as I understand, it may return a Failure which inherits MaybeObject and
not Object, thus the return type is MaybeObject*.

What syntax would you suggest for MaterializeScopeDetails?

[ulan, 2014-03-04 11:28:15]

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js#newco...
src/mirror-debugger.js:1941: function ScopeMirror(frame, function, index,
opt_details) {
On 2014/03/04 11:04:19, aandrey wrote:
> On 2014/03/04 10:41:59, ulan wrote:
> > Maybe just "details" instead of "opt_details". "opt" is confusing, it
probably
> > means "optional", but it can also meant "optimized" in v8. :)
> 
> Yes, it means "optional", and we do use "opt_" prefix all over the place (20+
> matches in this file).

In that case it is OK.

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12328
src/runtime.cc:12328: ScopeIterator it(isolate, frame, inlined_jsframe_index);
> You mean the JavaScriptFrame* pointer?
That pointer should be fine. This code works, but becomes confusing because of
mixing handles with maybe_object below.

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12330
src/runtime.cc:12330: MaybeObject* maybe_object =
MaterializeScopeDetails(isolate, &it);
Something like this:

Handle<Object> MaterializeScopeDetails(Isolate* isolate,     ScopeIterator* it).
It could return empty handle in the case where it is currently returning
Failure, then the caller could do

Handle<Object> scope_details = MaterializeScopeDetails(isolate, &it);
RETURN_IF_EMPTY_HANDLE(isolate, scope_details);
result.Add(scope_details, isolate);

This way it is clear that all the code is handlified.

[Yang, 2014-03-04 11:31:35]

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js
File src/mirror-debugger.js (right):

https://codereview.chromium.org/181063008/diff/1/src/mirror-debugger.js#newco...
src/mirror-debugger.js:1941: function ScopeMirror(frame, function, index,
opt_details) {
On 2014/03/04 11:04:19, aandrey wrote:
> On 2014/03/04 10:41:59, ulan wrote:
> > Maybe just "details" instead of "opt_details". "opt" is confusing, it
probably
> > means "optional", but it can also meant "optimized" in v8. :)
> 
> Yes, it means "optional", and we do use "opt_" prefix all over the place (20+
> matches in this file).

I think it's fine in this context.

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12308
src/runtime.cc:12308: // 1: Scope object
Now this seems just a broader case of %GetScopeDetails. Just an idea that you
may dismiss: we could share code and use an unified runtime function that takes
an optional scope index to distinguish between the two use cases.

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12328
src/runtime.cc:12328: ScopeIterator it(isolate, frame, inlined_jsframe_index);
On 2014/03/04 11:04:19, aandrey wrote:
> On 2014/03/04 10:41:59, ulan wrote:
> > This code seems to work, but it is fragile because it mixes handlified code
> with
> > raw pointers. ScopeIterator can cause GC, so after this point any raw
pointer
> to
> > v8 heap from above may become invalid.
> 
> You mean the JavaScriptFrame* pointer?
> I'm not an expert here, but this code, up to the point, is just a carbon copy
of
> the above Runtime_GetScopeDetails.
> 
> The JavaScriptFrame class is BASE_EMBEDDED. Should it care on GC being run?

Seems fine to me.

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12330
src/runtime.cc:12330: MaybeObject* maybe_object =
MaterializeScopeDetails(isolate, &it);
On 2014/03/04 11:04:19, aandrey wrote:
> On 2014/03/04 10:41:59, ulan wrote:
> > MaterializeScopeDetails looks already handlified, it could just return a
> handle,
> > instead of maybe_objects. The you wouldn't need checks and conversions
below.
> 
> As far as I understand, it may return a Failure which inherits MaybeObject and
> not Object, thus the return type is MaybeObject*.
> 
> What syntax would you suggest for MaterializeScopeDetails?

I suggest changing MaterializedScopeDetails to return a Handle<JSObject>. If the
scope_object is an empty handle, use RETURN_IF_EMPTY_HANDLE_VALUE to propagate
that empty handle to the caller. The caller should then check with
RETURN_IF_EMPTY_HANDLE.

I know it's not actually caused by this CL, but I since we are moving towards
less code with MaybeObject, this would be a nice small clean up.

[aandrey, 2014-03-04 12:39:07]

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/181063008/diff/1/src/runtime.cc#newcode12330
src/runtime.cc:12330: MaybeObject* maybe_object =
MaterializeScopeDetails(isolate, &it);
On 2014/03/04 11:31:35, Yang wrote:
> On 2014/03/04 11:04:19, aandrey wrote:
> > On 2014/03/04 10:41:59, ulan wrote:
> > > MaterializeScopeDetails looks already handlified, it could just return a
> > handle,
> > > instead of maybe_objects. The you wouldn't need checks and conversions
> below.
> > 
> > As far as I understand, it may return a Failure which inherits MaybeObject
and
> > not Object, thus the return type is MaybeObject*.
> > 
> > What syntax would you suggest for MaterializeScopeDetails?
> 
> I suggest changing MaterializedScopeDetails to return a Handle<JSObject>. If
the
> scope_object is an empty handle, use RETURN_IF_EMPTY_HANDLE_VALUE to propagate
> that empty handle to the caller. The caller should then check with
> RETURN_IF_EMPTY_HANDLE.
> 
> I know it's not actually caused by this CL, but I since we are moving towards
> less code with MaybeObject, this would be a nice small clean up.

Done.

[ulan, 2014-03-04 12:44:26]

lgtm

[ulan, 2014-03-07 11:03:42]

Committed patchset #2 manually as r19717 (presubmit successful).