Delay Class parsing until the class is actually used.

Delay Class parsing until the class is actually used.

Prior to this change the initial heap sizes were as follows:
ia32:
Size of isolate snapshot = 1230921
New space (0k of 32768k) Old space (1446k of 1604k)

X64:
Size of isolate snapshot = 1223943
New space (0k of 32768k) Old space (2630k of 2692k)


After this change the initial heap sizes are as follows:
ia32:
Size of isolate snapshot = 686443
New space (0k of 32768k) Old space (677k of 836k)

X64:
Size of isolate snapshot = 684731
New space (0k of 32768k) Old space (1220k of 1412k)

R=hausner@google.com, iposva@google.com, regis@google.com

Committed: https://code.google.com/p/dart/source/detail?r=23115

[hausner, 2013-05-21 20:07:30]

Some questions and comments.

I would add Regis to the list of reviewers, for the class finalization changes.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:109: 
Nit: extra blank line.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:175: cls.set_is_parsed();
I am slightly concerned about setting the 'is_parsed' flag before the class is
actually parsed. I think it's misleading. An alternative is to check the list
before you add a new class. I realize this is O(n^2) but will it be a problem in
practice?

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.h#new...
runtime/vm/compiler.h:27: // Extracts class, interface symbols from the script
and populates
Maybe remove the word interface while you are touching the line. This seems a
legacy reference.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.h#new...
runtime/vm/compiler.h:37: static RawError* ParseClass(const Class& cls);
We have Compile, CompileFunction, and ParseClass. Should this be CompileClass
instead?

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

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:2407: if (EnsureIsParsed(isolate) != Error::null()) {
How do errors during parsing get propagated to the caller?

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

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2498: void Parser::SkipToMatchingBraces() {
Parser::SkipBlock() does the same thing and additionally makes sure that all
braces, parens and brackets are properly nested.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3374: ErrorMsg("'%s' expected",
Token::Str(Token::kLBRACE));
Is it Rube Goldberg's birthday? ;-)
I think it's good enough to say ErrorMsg("'{' expected");
No need to use Token::Str() when we know what the token is.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3376: SkipToMatchingBraces();
Use SkipBlock instead (and maybe rename the function)?

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/snapshot.cc#ne...
runtime/vm/snapshot.cc:212: cls.EnsureIsParsed(isolate());
Does this mean you parse all classes that are in a snapshot?

[siva, 2013-05-23 00:54:30]

Addressed review comments.

Adding Regis to the review list as suggested by Mathias for the types
finalization part.

PTAL.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:109: 
On 2013/05/21 20:07:30, hausner wrote:
> Nit: extra blank line.

Done.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:175: cls.set_is_parsed();
I have renamed the is_parsed bit to is_marked_for_parsing and localized it's use
to within Compiler::ParseClass.

It is set at the beginning and reset at the end.

The alternative of repeatedly searching the array for the
class causes a slight degradation.


On 2013/05/21 20:07:30, hausner wrote:
> I am slightly concerned about setting the 'is_parsed' flag before the class is
> actually parsed. I think it's misleading. An alternative is to check the list
> before you add a new class. I realize this is O(n^2) but will it be a problem
in
> practice?

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.h#new...
runtime/vm/compiler.h:27: // Extracts class, interface symbols from the script
and populates
On 2013/05/21 20:07:30, hausner wrote:
> Maybe remove the word interface while you are touching the line. This seems a
> legacy reference.

Done.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/compiler.h#new...
runtime/vm/compiler.h:37: static RawError* ParseClass(const Class& cls);
On 2013/05/21 20:07:30, hausner wrote:
> We have Compile, CompileFunction, and ParseClass. Should this be CompileClass
> instead?

Done.

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

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:2407: if (EnsureIsParsed(isolate) != Error::null()) {
The function EnsureIsParsed does a Longjump on error if there is a jump context.
During parsing there is a jump context and this produces errors.

On 2013/05/21 20:07:30, hausner wrote:
> How do errors during parsing get propagated to the caller?

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

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:2498: void Parser::SkipToMatchingBraces() {
Thanks for pointing that out, used SkipBlock).

On 2013/05/21 20:07:30, hausner wrote:
> Parser::SkipBlock() does the same thing and additionally makes sure that all
> braces, parens and brackets are properly nested.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3374: ErrorMsg("'%s' expected",
Token::Str(Token::kLBRACE));
On 2013/05/21 20:07:30, hausner wrote:
> Is it Rube Goldberg's birthday? ;-)
> I think it's good enough to say ErrorMsg("'{' expected");
> No need to use Token::Str() when we know what the token is.

Done. 
(was just guarding for the time when we decide to use '@' to terminate blocks
;-) )

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/parser.cc#newc...
runtime/vm/parser.cc:3376: SkipToMatchingBraces();
On 2013/05/21 20:07:30, hausner wrote:
> Use SkipBlock instead (and maybe rename the function)?

Done.

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/14820028/diff/28001/runtime/vm/snapshot.cc#ne...
runtime/vm/snapshot.cc:212: cls.EnsureIsParsed(isolate());
No this code is only used in isolate messages. When we send objects over we need
to make sure the other end has parsed the class.

On 2013/05/21 20:07:30, hausner wrote:
> Does this mean you parse all classes that are in a snapshot?

[hausner, 2013-05-23 04:12:24]

LGTM but you may want to wait for Regis' verdict about class finalization.

[Ivan Posva, 2013-05-23 05:10:37]

Let's discuss the work list issue in person.

-Ivan

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:187: for (intptr_t i = 0; i < parse_list.Length(); i++) {
This is a very unusual way to do fix-point iteration.

Reading the code straight line you wonder why this would loop more than once. As
only one element has been added to parse_list when entering the first time.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.h#new...
runtime/vm/compiler.h:34: // the symbol tables and the class dictionary of the
library.
This comment is somewhat confusing. Wasn't the class dictionary already
populated above in Compile? If not, how did you find this class to compile?

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:146: Parser(const Script& script, const Library& library,
intptr_t token_pos);
Do these constructors need to be public?

[regis, 2013-05-23 16:50:04]

Type finalization changes LGTM.

I would try to generate a couple of snapshots specifying the 
error_on_malformed_type flag to make sure types are properly finalized.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:686: OS::Print("Finalize type '%s' for class
%s\n",
Wrap names in single quotes: '%s'

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1287: OS::Print("done mixin type appl %s %s
extending %s\n",
Wrap %s in single quotes.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1473: // member types in order to break cycles.
The comment still mentions ' member types', but you postpone this phase to
FinalizeClass.

[siva, 2013-05-23 20:19:45]

--error_on_malformed_type is always on by default when generating snapshots.

Thanks for the reviews.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:686: OS::Print("Finalize type '%s' for class
%s\n",
On 2013/05/23 16:50:04, regis wrote:
> Wrap names in single quotes: '%s'

Done.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1287: OS::Print("done mixin type appl %s %s
extending %s\n",
On 2013/05/23 16:50:04, regis wrote:
> Wrap %s in single quotes.

Done.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:1473: // member types in order to break cycles.
On 2013/05/23 16:50:04, regis wrote:
> The comment still mentions ' member types', but you postpone this phase to
> FinalizeClass.

Done.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.cc#ne...
runtime/vm/compiler.cc:187: for (intptr_t i = 0; i < parse_list.Length(); i++) {
Added a comment with an uppercase NOTE to make sure people don't mis understand
this.

On 2013/05/23 05:10:37, Ivan Posva wrote:
> This is a very unusual way to do fix-point iteration.
> 
> Reading the code straight line you wonder why this would loop more than once.
As
> only one element has been added to parse_list when entering the first time.

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.h
File runtime/vm/compiler.h (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/compiler.h#new...
runtime/vm/compiler.h:34: // the symbol tables and the class dictionary of the
library.
Changed as discussed offline.

On 2013/05/23 05:10:37, Ivan Posva wrote:
> This comment is somewhat confusing. Wasn't the class dictionary already
> populated above in Compile? If not, how did you find this class to compile?

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/14820028/diff/38001/runtime/vm/parser.h#newco...
runtime/vm/parser.h:146: Parser(const Script& script, const Library& library,
intptr_t token_pos);
Made them private.

On 2013/05/23 05:10:37, Ivan Posva wrote:
> Do these constructors need to be public?

[Ivan Posva, 2013-05-23 22:33:36]

LGTM -ip

[siva, 2013-05-24 00:13:18]

Committed patchset #6 manually as r23115 (presubmit successful).