More precompiled snapshotting.

More precompiled snapshotting.

- Load precompiled stub code from the snapshot.
- Mark isolates reading from precompiled snapshots as not allowed to compile.
- Reset static field values during precompilation.
- Don't run the program after generating a precompiled snapshot.

Fails sooner in loading precompiled snapshots due to null Instructions.

R=asiva@google.com

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

[rmacnak, 2015-09-05 00:10:19]

rmacnak@google.com changed reviewers:
+ asiva@google.com

[Florian Schneider, 2015-09-05 08:38:55]

On 2015/09/05 00:10:20, rmacnak wrote:

dbc: I guess resetting statics is not needed yet for precompiling unoptimized
code, but only when we want to put optimized code into the snapshot. In the
first case, there won't be any code executed before writing out the code to the
snapshot.

Should we start adding features to optimized code in snapshot already, or land
them later separately? I'm a little in favor of smaller CLs for now.

[rmacnak, 2015-09-09 00:46:40]

On 2015/09/05 08:38:55, Florian Schneider wrote:
> On 2015/09/05 00:10:20, rmacnak wrote:
> 
> dbc: I guess resetting statics is not needed yet for precompiling unoptimized
> code, but only when we want to put optimized code into the snapshot. In the
> first case, there won't be any code executed before writing out the code to
the
> snapshot.
> 
> Should we start adding features to optimized code in snapshot already, or land
> them later separately? I'm a little in favor of smaller CLs for now.

Some loading code is in Dart and runs before the snapshot is taken. Supporting
full optimized code isn't the intent here.

[rmacnak, 2015-09-14 19:17:01]

PTAL

[siva, 2015-09-14 20:32:59]

https://codereview.chromium.org/1311403009/diff/60001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://codereview.chromium.org/1311403009/diff/60001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1001: Log::PrintErr("Error: Failed to write snapshot
file.\n\n");
This should be:

      ErrorExit(kErrorExitCode,
                "Unable to open file %s for writing the snapshot\n",
                filename);

https://codereview.chromium.org/1311403009/diff/60001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1018: exit(kErrorExitCode);
why not ErrorExit ?

https://codereview.chromium.org/1311403009/diff/60001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1028: exit(kErrorExitCode);
why not ErrorExit ?

https://codereview.chromium.org/1311403009/diff/60001/runtime/bin/main.cc#new...
runtime/bin/main.cc:1033: exit(kErrorExitCode);
Ditto.

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

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:145: }
Would read better as:

// When precompiling the stub code is initialized from the snapshot.
if (!precompiled) {
  StubCode::InitOnce();
}

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/dart.cc#newc...
runtime/vm/dart.cc:172: Symbols::InitOnce(vm_isolate_);
Maybe the if (!precompiled) check from above should go here, isn;t it true that
if precompiling (vm_isolate_snapshot != NULL) should be true.

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/megamorphic_...
File runtime/vm/megamorphic_cache_table.h (right):

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/megamorphic_...
runtime/vm/megamorphic_cache_table.h:39: void WriteTo(SnapshotWriter* writer);
As discussed offline if the megamorphic_cache_table is converted from a C heap
object to a dart Object it would not be necessary to have these methods here.

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

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:446: }
can we assert here that
Object::instructions_snapshot_buffer() is NULL
that we now that the invariant of the initializer field being used for saving
the static initial value is not done when running from precompiled code.

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/raw_object_s...
File runtime/vm/raw_object_snapshot.cc (right):

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/raw_object_s...
runtime/vm/raw_object_snapshot.cc:773:
writer->WriteObjectImpl(ptr()->initializer_.precompiled_, kAsReference);
should this also be conditional under
if (writer->snapshot_code()) {
 .... precompiled_,
} else {
 .... saved_value_,
}
for better readability.

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

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1550: StubCode::name##_entry_ = new
StubEntry(*CodeHandle());
Instead of exposing the stub code macro order here it might be better to have a
StubCode::InitOnceFromSnapshot() function which will suck in the instructions
for the various stubs and then create code objects and finalize the stubs.

This would also mean that we could continue to restrict HandleVmIsolateObjects
to just the specific ones we have instead of opening it up for all generic
objects.

(Maybe we can do this cleanup in another CL).

https://codereview.chromium.org/1311403009/diff/60001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:2006: }
Why do you order this in between the object_store objects and it's forward
references?
Maybe this is moot once you make the megamorphic cache table into a regular dart
object and stick it into the object store.

[rmacnak, 2015-09-15 17:56:42]

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/bin/main.cc
File runtime/bin/main.cc (right):

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/bin/main...
runtime/bin/main.cc:1001: Log::PrintErr("Error: Failed to write snapshot
file.\n\n");
On 2015/09/14 20:32:59, siva wrote:
> This should be:
> 
>       ErrorExit(kErrorExitCode,
>                 "Unable to open file %s for writing the snapshot\n",
>                 filename);

Done.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/bin/main...
runtime/bin/main.cc:1018: exit(kErrorExitCode);
On 2015/09/14 20:32:59, siva wrote:
> why not ErrorExit ?

Done.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/bin/main...
runtime/bin/main.cc:1028: exit(kErrorExitCode);
On 2015/09/14 20:32:59, siva wrote:
> why not ErrorExit ?

Done.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/bin/main...
runtime/bin/main.cc:1033: exit(kErrorExitCode);
On 2015/09/14 20:32:59, siva wrote:
> Ditto.

Done.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/dart.cc
File runtime/vm/dart.cc (right):

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/dart....
runtime/vm/dart.cc:145: }
On 2015/09/14 20:32:59, siva wrote:
> Would read better as:
> 
> // When precompiling the stub code is initialized from the snapshot.
> if (!precompiled) {
>   StubCode::InitOnce();
> }

Done.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/dart....
runtime/vm/dart.cc:172: Symbols::InitOnce(vm_isolate_);
On 2015/09/14 20:32:59, siva wrote:
> Maybe the if (!precompiled) check from above should go here, isn;t it true
that
> if precompiling (vm_isolate_snapshot != NULL) should be true.

Yes, precompiled implies there is a vm isolate snapshot.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/preco...
File runtime/vm/precompiler.cc (right):

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/preco...
runtime/vm/precompiler.cc:446: }
On 2015/09/14 20:32:59, siva wrote:
> can we assert here that
> Object::instructions_snapshot_buffer() is NULL
> that we now that the invariant of the initializer field being used for saving
> the static initial value is not done when running from precompiled code. 

Done.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/raw_o...
File runtime/vm/raw_object_snapshot.cc (right):

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/raw_o...
runtime/vm/raw_object_snapshot.cc:773:
writer->WriteObjectImpl(ptr()->initializer_.precompiled_, kAsReference);
On 2015/09/14 20:32:59, siva wrote:
> should this also be conditional under
> if (writer->snapshot_code()) {
>  .... precompiled_,
> } else {
>  .... saved_value_,
> }
> for better readability.

Done.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/snaps...
File runtime/vm/snapshot.cc (right):

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/snaps...
runtime/vm/snapshot.cc:1550: StubCode::name##_entry_ = new
StubEntry(*CodeHandle());
On 2015/09/14 20:32:59, siva wrote:
> Instead of exposing the stub code macro order here it might be better to have
a
> StubCode::InitOnceFromSnapshot() function which will suck in the instructions
> for the various stubs and then create code objects and finalize the stubs.
> 
> This would also mean that we could continue to restrict HandleVmIsolateObjects
> to just the specific ones we have instead of opening it up for all generic
> objects.
> 
> (Maybe we can do this cleanup in another CL).

Pushed to stub_code.cc and added TODO.

https://chromiumcodereview.appspot.com/1311403009/diff/60001/runtime/vm/snaps...
runtime/vm/snapshot.cc:2006: }
On 2015/09/14 20:32:59, siva wrote:
> Why do you order this in between the object_store objects and it's forward
> references?
> Maybe this is moot once you make the megamorphic cache table into a regular
dart
> object and stick it into the object store.

Dropped.

[siva, 2015-09-15 23:21:03]

lgtm

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

https://codereview.chromium.org/1311403009/diff/100001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:1571:
Isolate::Current()->set_compilation_allowed(instructions_buffer_ == NULL);
isolate()->set_compilation_allowed(...);

[rmacnak, 2015-09-15 23:53:21]

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

https://codereview.chromium.org/1311403009/diff/100001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:1571:
Isolate::Current()->set_compilation_allowed(instructions_buffer_ == NULL);
On 2015/09/15 23:21:02, siva wrote:
> isolate()->set_compilation_allowed(...);

Done.

[rmacnak, 2015-09-15 23:53:48]

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