Added full deferred loading semantic to precompiled/--noopt/eager-loading code

Added full deferred loading semantic to precompiled/--noopt/eager-loading code (some corner cases, e.g., compile-time errors for constants, still missing)

BUG=
R=hausner@google.com

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

[srdjan, 2015-07-08 16:53:50]

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

[hausner, 2015-07-08 18:19:58]

Apart from deferred constants not working, how about allocation of deferred
types?

new prefix.T() needs some check at runtime whether the prefix is loaded.

Another subtlety is handling prefix.loadLibrary() correctly if the library
exports that name as well. Before the library is loaded, loadLibrary refers to
the method of the prefix. After loading, it refers to the exported name of the
library.

https://codereview.chromium.org/1211273011/diff/40001/runtime/lib/lib_prefix....
File runtime/lib/lib_prefix.dart (right):

https://codereview.chromium.org/1211273011/diff/40001/runtime/lib/lib_prefix....
runtime/lib/lib_prefix.dart:13: bool is_loaded() native
"LibraryPrefix_isLoaded";
Should this be isLoaded() so it conforms to the Dart convention?

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

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10371: // When eagerely loading classes we emit runtime
checks for loadef state of
loadef -> loaded.

I would change the compiler so dependent code does not get registered in the
first place when loading eagerly. I think that's less hacky. Just assert here
that we are not loading eagerly, and that we're not running with precompiled
code.

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

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11830: if (prefix.is_loaded()) {
This should maybe be prefix.is_loaded() || FLAG_load_deferred_eagerly

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11837: parsed_function()->AddDeferredPrefix(prefix);
It's not necessary (or even wrong) to make code dependent on a prefix when
loading eagerly. The code never gets invalidated since the library is loaded
eagerly. If it did get invalidated, the function would have to be compiled
again, which is not possible in a pre-compiled situation.

You should probably put an assertion in AddDeferredPrefix or some more
appropriate place to ensure that it's not used with precompiled code.

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:13109:
current_block_->statements->Add(ThrowNoSuchMethodError(
Can you add a comment here that reminds that adding a statement to current_block
is a hack. Parsing an expression should actually not have a side effect on the
sequence node of the current block.

[srdjan, 2015-07-09 16:41:44]

PTAL.
There are still some failing deferred loading tests, which I plan to look at in
follow-up CLs.

https://codereview.chromium.org/1211273011/diff/40001/runtime/lib/lib_prefix....
File runtime/lib/lib_prefix.dart (right):

https://codereview.chromium.org/1211273011/diff/40001/runtime/lib/lib_prefix....
runtime/lib/lib_prefix.dart:13: bool is_loaded() native
"LibraryPrefix_isLoaded";
On 2015/07/08 18:19:57, hausner wrote:
> Should this be isLoaded() so it conforms to the Dart convention?

Done.

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

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10371: // When eagerely loading classes we emit runtime
checks for loadef state of
On 2015/07/08 18:19:57, hausner wrote:
> loadef -> loaded.
> 
> I would change the compiler so dependent code does not get registered in the
> first place when loading eagerly. I think that's less hacky. Just assert here
> that we are not loading eagerly, and that we're not running with precompiled
> code.

No deferred prefixes are allowed when eager deferred loading is enabled.
Added an assert in 'DisableCode'

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

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11830: if (prefix.is_loaded()) {
On 2015/07/08 18:19:58, hausner wrote:
> This should maybe be prefix.is_loaded() || FLAG_load_deferred_eagerly

Done.

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11837: parsed_function()->AddDeferredPrefix(prefix);
On 2015/07/08 18:19:58, hausner wrote:
> It's not necessary (or even wrong) to make code dependent on a prefix when
> loading eagerly. The code never gets invalidated since the library is loaded
> eagerly. If it did get invalidated, the function would have to be compiled
> again, which is not possible in a pre-compiled situation.
> 
> You should probably put an assertion in AddDeferredPrefix or some more
> appropriate place to ensure that it's not used with precompiled code.

By adding '|| FLAG_load_deferred_eagerly' above as suggested, this code is not
executed any longer, deferred prefixes are not added.

https://codereview.chromium.org/1211273011/diff/40001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:13109:
current_block_->statements->Add(ThrowNoSuchMethodError(
On 2015/07/08 18:19:58, hausner wrote:
> Can you add a comment here that reminds that adding a statement to
current_block
> is a hack. Parsing an expression should actually not have a side effect on the
> sequence node of the current block.

Done.

[hausner, 2015-07-09 17:10:05]

LGTM w/comments

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

https://codereview.chromium.org/1211273011/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10051: String& method_name = String::Handle();
String::Handle(Z);

https://codereview.chromium.org/1211273011/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10088: String& method_name = String::Handle();
String::Handle(Z);

https://codereview.chromium.org/1211273011/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11990: RawAbstractType* Parser::ParseTypePrefix(
The name ParseTypePrefix confused me at first. It sounds like only the prefix
gets parsed. Maybe it's ok to overload the ParsePrefix() function, with and
without the prefix.

[srdjan, 2015-07-09 17:31:19]

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

https://codereview.chromium.org/1211273011/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10051: String& method_name = String::Handle();
On 2015/07/09 17:10:05, hausner wrote:
> String::Handle(Z);

Done.

https://codereview.chromium.org/1211273011/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:10088: String& method_name = String::Handle();
On 2015/07/09 17:10:05, hausner wrote:
> String::Handle(Z);

Done.

https://codereview.chromium.org/1211273011/diff/60001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:11990: RawAbstractType* Parser::ParseTypePrefix(
On 2015/07/09 17:10:05, hausner wrote:
> The name ParseTypePrefix confused me at first. It sounds like only the prefix
> gets parsed. Maybe it's ok to overload the ParsePrefix() function, with and
> without the prefix.

Done.

[srdjan, 2015-07-09 17:57:14]

Committed patchset #7 (id:120001) manually as
d503d2f0ed3ae1f8ab41c9749afbc3c41ee8421e (presubmit successful).