Eliminate all but one top-level class per library.

Eliminate all but one top-level class per library.

Each script has an associated top-level class in which the top-level functions and fields are stored, and the reference to the script. All other fields in the TL class are unused. There are 380 TL classes in corelib alone; eliminating them saves space.

This CL eliminates all TL classes but one per library. All TL functions and fields in the library are stored in that TL class. Because scripts are not stored directly in functions and fields, but are accessible via their owner class, the owner of TL entities are now PatchClasses, rather than classes.

Before:
Size of vm isolate snapshot = 930813
New space (0k of 0k) Old space (1184k of 1624k)
VM Isolate: Number of symbols : 14909
Size of isolate snapshot = 261873
New space (0k of 2048k) Old space (987k of 1024k)

After:
Size of vm isolate snapshot = 931101
New space (0k of 0k) Old space (713k of 1156k)
VM Isolate: Number of symbols : 14907
Size of isolate snapshot = 256956
New space (0k of 1024k) Old space (514k of 768k)

R=iposva@google.com

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

[hausner, 2015-11-19 18:33:37]

Description was changed from

==========
Attempt to eliminate all but one top-level class per library.

This CL isn’t really working because the VM makes the assumption
that a closure function in a class (including top-level classes)
can be identified by a token position:

Class::LookupClosureFunction(token_pos)

The VM also tries to look up functions in a script by token position

Library::LookupFunctionInScript(script, token_pos)
Class::LookupFunctionAtToken(token_pos)

That no longer works when there isn’t one class per script.


Mysteriously, the snapshot size is not significantly smaller with this
change, even though it eliminates over 300 top-level classes in core lib
alone.


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

to

==========
Attempt to eliminate all but one top-level class per library.

This CL isn’t really working because the VM makes the assumption
that a closure function in a class (including top-level classes)
can be identified by a token position:

Class::LookupClosureFunction(token_pos)

The VM also tries to look up functions in a script by token position

Library::LookupFunctionInScript(script, token_pos)
Class::LookupFunctionAtToken(token_pos)

That no longer works when there isn’t one class per script.

Also, Metadata has the same issue. We store the owner class and a token position
in the metadata field. This has to change to a patch class owner and token
position.

Mysteriously, the snapshot size is not significantly smaller with this
change, even though it eliminates over 300 top-level classes in core lib
alone.


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

[hausner, 2015-11-19 18:33:59]

Description was changed from

==========
Attempt to eliminate all but one top-level class per library.

This CL isn’t really working because the VM makes the assumption
that a closure function in a class (including top-level classes)
can be identified by a token position:

Class::LookupClosureFunction(token_pos)

The VM also tries to look up functions in a script by token position

Library::LookupFunctionInScript(script, token_pos)
Class::LookupFunctionAtToken(token_pos)

That no longer works when there isn’t one class per script.

Also, Metadata has the same issue. We store the owner class and a token position
in the metadata field. This has to change to a patch class owner and token
position.

Mysteriously, the snapshot size is not significantly smaller with this
change, even though it eliminates over 300 top-level classes in core lib
alone.


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

to

==========
Attempt to eliminate all but one top-level class per library.

This CL isn’t really working because the VM makes the assumption
that a closure function in a class (including top-level classes)
can be identified by a token position:

Class::LookupClosureFunction(token_pos)

The VM also tries to look up functions in a script by token position

Library::LookupFunctionInScript(script, token_pos)
Class::LookupFunctionAtToken(token_pos)

That no longer works when there isn’t one class per script.

Also, Metadata has the same issue. We store the owner class and a token
position in the metadata field. This has to change to a patch class owner
and token position.

Mysteriously, the snapshot size is not significantly smaller with this
change, even though it eliminates over 300 top-level classes in core lib
alone.


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

[hausner, 2015-11-20 21:18:10]

Description was changed from

==========
Attempt to eliminate all but one top-level class per library.

This CL isn’t really working because the VM makes the assumption
that a closure function in a class (including top-level classes)
can be identified by a token position:

Class::LookupClosureFunction(token_pos)

The VM also tries to look up functions in a script by token position

Library::LookupFunctionInScript(script, token_pos)
Class::LookupFunctionAtToken(token_pos)

That no longer works when there isn’t one class per script.

Also, Metadata has the same issue. We store the owner class and a token
position in the metadata field. This has to change to a patch class owner
and token position.

Mysteriously, the snapshot size is not significantly smaller with this
change, even though it eliminates over 300 top-level classes in core lib
alone.


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

to

==========
Eliminate all but one top-level class per library.

Each script has an associated top-level class in which the top-level functions
and fields are stored, and the reference to the script. All other fields in the
TL class are unused. There are 380 TL classes in corelib alone; eliminating them
saves space.

This CL eliminates all TL classes but one per library. All TL functions and
fields in the library are stored in that TL class. Because scripts are not
stored directly in functions and fields, but are accessible via their owner
class, the owner of TL entities are now PatchClasses, rather than classes.
==========

[hausner, 2015-11-20 21:18:10]

hausner@google.com changed reviewers:
+ iposva@google.com

[hausner, 2015-11-20 21:18:43]

All tests pass.

[hausner, 2015-11-20 23:41:44]

Description was changed from

==========
Eliminate all but one top-level class per library.

Each script has an associated top-level class in which the top-level functions
and fields are stored, and the reference to the script. All other fields in the
TL class are unused. There are 380 TL classes in corelib alone; eliminating them
saves space.

This CL eliminates all TL classes but one per library. All TL functions and
fields in the library are stored in that TL class. Because scripts are not
stored directly in functions and fields, but are accessible via their owner
class, the owner of TL entities are now PatchClasses, rather than classes.
==========

to

==========
Eliminate all but one top-level class per library.

Each script has an associated top-level class in which the top-level functions
and fields are stored, and the reference to the script. All other fields in the
TL class are unused. There are 380 TL classes in corelib alone; eliminating them
saves space.

This CL eliminates all TL classes but one per library. All TL functions and
fields in the library are stored in that TL class. Because scripts are not
stored directly in functions and fields, but are accessible via their owner
class, the owner of TL entities are now PatchClasses, rather than classes.

Before:
Size of vm isolate snapshot = 930813
New space (0k of 0k) Old space (1184k of 1624k)
VM Isolate: Number of symbols : 14909
Size of isolate snapshot = 261873
New space (0k of 2048k) Old space (987k of 1024k)

After:
Size of vm isolate snapshot = 931101
New space (0k of 0k) Old space (713k of 1156k)
VM Isolate: Number of symbols : 14907
Size of isolate snapshot = 256956
New space (0k of 1024k) Old space (514k of 768k)
==========

[hausner, 2015-11-20 23:42:42]

Added before and after isolate data to the CL description.

[Ivan Posva, 2015-11-24 21:30:12]

Small comments we collected while trying to find a suitable name for RawOwner.

-Ivan

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:1722: RawClass* source_class() const { return
raw_ptr()->source_class_; }
origin_class() or just origin()?

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:2143: RawClass* origin() const;
Origin()

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:2144: RawScript* script() const;
Script()

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:2894: RawClass* owner() const;
Owner(), Origin(), Script()

[hausner, 2015-11-24 22:45:35]

First round of review comments addressed.

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:1722: RawClass* source_class() const { return
raw_ptr()->source_class_; }
On 2015/11/24 21:30:11, Ivan Posva wrote:
> origin_class() or just origin()?

origin_class(). Done.

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:2143: RawClass* origin() const;
On 2015/11/24 21:30:12, Ivan Posva wrote:
> Origin()

Leaving origin(), as discussed offline.

https://codereview.chromium.org/1410383020/diff/40001/runtime/vm/object.h#new...
runtime/vm/object.h:2144: RawScript* script() const;
On 2015/11/24 21:30:12, Ivan Posva wrote:
> Script()

Leaving lower-case due to name conflict with class Script.

[Ivan Posva, 2015-11-30 23:54:27]

LGTM with optimizations in a follow-up CL.

-Ivan

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9417: field = Field::NewTopLevel(metaname,
"= Field::Handle(Field::NewTopLevel(...)" so that we can make the handle const.

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.h#new...
runtime/vm/object.h:2145: RawObject* RawOwner() const { return
raw_ptr()->owner_; }
I really would prefer smrck() over RawOwner.

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.h#new...
runtime/vm/object.h:2897: RawObject* RawOwner() const { return
raw_ptr()->owner_; }
ditto

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:1854: sig_func_owner = PatchClass::New(current_class(),
script_);
If the current_class().script and script_.raw() are not the same, should we not
reuse the PatchClass object that was used to setup this disparity?

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6137: library_.set_toplevel_class(toplevel_class);
For this compilation unit we can use this top level class and do not need to
allocate a separate patch class. This would be an additional optimization.
Different CL maybe?

[hausner, 2015-12-01 17:22:04]

Description was changed from

==========
Eliminate all but one top-level class per library.

Each script has an associated top-level class in which the top-level functions
and fields are stored, and the reference to the script. All other fields in the
TL class are unused. There are 380 TL classes in corelib alone; eliminating them
saves space.

This CL eliminates all TL classes but one per library. All TL functions and
fields in the library are stored in that TL class. Because scripts are not
stored directly in functions and fields, but are accessible via their owner
class, the owner of TL entities are now PatchClasses, rather than classes.

Before:
Size of vm isolate snapshot = 930813
New space (0k of 0k) Old space (1184k of 1624k)
VM Isolate: Number of symbols : 14909
Size of isolate snapshot = 261873
New space (0k of 2048k) Old space (987k of 1024k)

After:
Size of vm isolate snapshot = 931101
New space (0k of 0k) Old space (713k of 1156k)
VM Isolate: Number of symbols : 14907
Size of isolate snapshot = 256956
New space (0k of 1024k) Old space (514k of 768k)
==========

to

==========
Eliminate all but one top-level class per library.

Each script has an associated top-level class in which the top-level functions
and fields are stored, and the reference to the script. All other fields in the
TL class are unused. There are 380 TL classes in corelib alone; eliminating them
saves space.

This CL eliminates all TL classes but one per library. All TL functions and
fields in the library are stored in that TL class. Because scripts are not
stored directly in functions and fields, but are accessible via their owner
class, the owner of TL entities are now PatchClasses, rather than classes.

Before:
Size of vm isolate snapshot = 930813
New space (0k of 0k) Old space (1184k of 1624k)
VM Isolate: Number of symbols : 14909
Size of isolate snapshot = 261873
New space (0k of 2048k) Old space (987k of 1024k)

After:
Size of vm isolate snapshot = 931101
New space (0k of 0k) Old space (713k of 1156k)
VM Isolate: Number of symbols : 14907
Size of isolate snapshot = 256956
New space (0k of 1024k) Old space (514k of 768k)

R=iposva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/ad0f14e0ce54efb62a297b259156face55708100
==========

[hausner, 2015-12-01 17:22:05]

Committed patchset #5 (id:80001) manually as
ad0f14e0ce54efb62a297b259156face55708100 (presubmit successful).

[hausner, 2015-12-01 19:38:17]

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9417: field = Field::NewTopLevel(metaname,
On 2015/11/30 23:54:26, Ivan Posva wrote:
> "= Field::Handle(Field::NewTopLevel(...)" so that we can make the handle
const.

Done.

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.h#new...
runtime/vm/object.h:2145: RawObject* RawOwner() const { return
raw_ptr()->owner_; }
On 2015/11/30 23:54:26, Ivan Posva wrote:
> I really would prefer smrck() over RawOwner.

Or sugus()?

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/object.h#new...
runtime/vm/object.h:2897: RawObject* RawOwner() const { return
raw_ptr()->owner_; }
On 2015/11/30 23:54:26, Ivan Posva wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:1854: sig_func_owner = PatchClass::New(current_class(),
script_);
On 2015/11/30 23:54:26, Ivan Posva wrote:
> If the current_class().script and script_.raw() are not the same, should we
not
> reuse the PatchClass object that was used to setup this disparity?

Yes. But unless I add the RawOwner to the Parser class, we don't have access to
it here. This code can be called before the Function object (whose parameters we
are parsing) exists. Thus, I can't 'steal' the owner from it.

We only get to this code when we parse a function type that is 'inlined'
directly in the signature of a function. I don't expect that to be common. Most
code would use a typedef for that. Thus, I thought we can get away with
allocating a patch class in this rare case.

https://codereview.chromium.org/1410383020/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:6137: library_.set_toplevel_class(toplevel_class);
On 2015/11/30 23:54:27, Ivan Posva wrote:
> For this compilation unit we can use this top level class and do not need to
> allocate a separate patch class. This would be an additional optimization.
> Different CL maybe?

Done.