Collect closure functions in isolate

Collect closure functions in isolate

Instead of collecting closures functions in each class object, maintain one list per isolate. This is a step towards getting rid of top-level classes.

I'd appreciate if John could take a look at the service isolate and coverage related change.

I'd appreciate if Ryan could take a look at the precompilation related change.

When compiling all of corelib, the list of closures in the isolate is about 600 entries long. If this linear list should become a bottleneck, I'll deal with it later. (Sadly, some code relies on the fact that a closure can be identified with a list index, so making it a hash table instead of an array does not work.)

BUG=
R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/648e52baa891816fc887b50e1096a048d9fb206e

[hausner, 2015-11-13 01:23:30]

Description was changed from

==========
Collect closure functions in isolate

BUG=
==========

to

==========
Collect closure functions in isolate

Instead of collecting closures functions in each class object, maintain one list
per isolate. This is a step towards getting rid of top-level classes.

I'd appreciate if John could take a look at the service isolate and coverage
related change.

I'd appreciate if Ryan could take a look at the precompilation related change.

When compiling all of corelib, the list of closures in the isolate is about 600
entries long. If this linear list should become a bottleneck, I'll deal with it
later. (Sadly, some code relies on the fact that a closure can be identified
with a list index, so making it a hash table instead of an array does not work.)

BUG=
==========

[hausner, 2015-11-13 01:23:30]

hausner@google.com changed reviewers:
+ iposva@google.com, johnmccutchan@google.com, rmacnak@google.com

[Ivan Posva, 2015-11-13 03:53:07]

On 2015/11/13 01:23:30, hausner wrote:
> Description was changed from
> 
> ==========
> Collect closure functions in isolate
> 
> BUG=
> ==========
> 
> to
> 
> ==========
> Collect closure functions in isolate
> 
> Instead of collecting closures functions in each class object, maintain one
list
> per isolate. This is a step towards getting rid of top-level classes.
> 
> I'd appreciate if John could take a look at the service isolate and coverage
> related change.
> 
> I'd appreciate if Ryan could take a look at the precompilation related change.
> 
> When compiling all of corelib, the list of closures in the isolate is about
600
> entries long. If this linear list should become a bottleneck, I'll deal with
it
> later. (Sadly, some code relies on the fact that a closure can be identified
> with a list index, so making it a hash table instead of an array does not
work.)

We could also implement the fixed-order iteration maps in C++. Actually, I think
we should implement the fixed-order iteration maps in C++ and make them
identical to the Dart versions. Then this issue will be easily addressed.

Thanks,
-Ivan

[hausner, 2015-11-13 17:35:01]

For an ia32 build, this change reduces the VM snapshot by about 1.1 KB and the
old space VM heap by 9 KB. I presume this is because of the eliminated field in
RawClass.

[Ivan Posva, 2015-11-13 17:47:51]

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1882: closures =
isolate_->object_store()->closure_functions();
Why did this code move from after the class table based lookup to before?

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1967: closures =
isolate_->object_store()->closure_functions();
ditto

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2829: // in mixin classes whose source code is contained
in a different
I don't think this comment is entirely right anymore. Mixins or not don't really
matter now.

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2503: (found_func.parent_function() !=
function.parent_function())) {
Why would any of these tests for pos or script fail after we have looked up and
found the closure function?

Also it looks like we could be repeatedly entering a closure functions if the
one we find first does not have the same parent due to the parent being
closurized or not.

[Cutch, 2015-11-13 17:51:58]

The service protocol does not need to use indexes to look objects up in the
heap. The protocol treats an object's id as an opaque string and the VM splits
that string into pieces when mapping from id to object. For example, currently,
we use class id to lookup classes but we could use class name + library name or
something else.

[rmacnak, 2015-11-13 18:07:19]

tree shaker LGTM

[hausner, 2015-11-13 20:13:29]

Address iposva's review comments.

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1882: closures =
isolate_->object_store()->closure_functions();
On 2015/11/13 17:47:51, Ivan Posva wrote:
> Why did this code move from after the class table based lookup to before?

It wasn't after the class table, it was inside the loop over the class table,
since closures were kept in class objects. It had to move outside of the loop,
either before or after. I have no specific preference.

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:1967: closures =
isolate_->object_store()->closure_functions();
On 2015/11/13 17:47:51, Ivan Posva wrote:
> ditto

ditto

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2829: // in mixin classes whose source code is contained
in a different
On 2015/11/13 17:47:51, Ivan Posva wrote:
> I don't think this comment is entirely right anymore. Mixins or not don't
really
> matter now.

True. The script needs to be checked now because of a different reason. Removed
comment altogether, it is no longer needed.

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/flow_graph_b...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/1436243005/diff/20001/runtime/vm/flow_graph_b...
runtime/vm/flow_graph_builder.cc:2503: (found_func.parent_function() !=
function.parent_function())) {
On 2015/11/13 17:47:51, Ivan Posva wrote:
> Why would any of these tests for pos or script fail after we have looked up
and
> found the closure function?
> 
> Also it looks like we could be repeatedly entering a closure functions if the
> one we find first does not have the same parent due to the parent being
> closurized or not.

It seems that LookupClosureFunction was implemented to find the innermost
closure containing a certain token position, yet all call sites are interested
in the closure starting at a given token position. (The debugger searches for
innermost closures, but it has its own functions to do so.)

I changed LookupClosureFunction and adapted all call sites.

[hausner, 2015-11-17 23:40:31]

Committed patchset #6 (id:100001) manually as
648e52baa891816fc887b50e1096a048d9fb206e (presubmit successful).

[hausner, 2015-11-17 23:41:51]

We discussed this change offline. Will address additional review comments in a
later checkin.

TBR/LGTM'ed