Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
 - Use the type of isolate we are writing instead of an object's current isolate to decide if vm isolate objects are written symbolically.

BUG=
R=asiva@google.com

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

[rmacnak, 2016-05-04 01:19:25]

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

[rmacnak, 2016-05-04 22:54:18]

Description was changed from

==========
Towards taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Ensure regular isolates always have the VM isolate's predefined symbols in
their own symbol table, and remove the two-phase symbol lookup as a result.
 - Fix symbol table compaction: We want to ensure a compacted symbol table has
all the predefined symbols even if they aren't currently used, and we should
expect to add them from the VM isolate not the competing ones from the current
isolate.
 - Fix confused treatment of references to vm isolate objects as non-symbolic
when writing a precompiled regular isolate snapshot.
==========

to

==========
Towards taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Fix confused treatment of references to vm isolate objects as non-symbolic
when writing a precompiled regular isolate snapshot.

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

[rmacnak, 2016-05-04 23:14:47]

Updated to keep the small table in the regular isolate and two phase symbol
lookup. PTAL.

[siva, 2016-05-05 00:50:05]

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

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:1931: }
Instead of this code which is heard to read and understand here why not filer
out calls to HandleVMIsolateObject based on the cid at the caller location.

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:2286: if (rawobj->IsVMHeapObject() &&
HandleVMIsolateObject(rawobj)) {
You could convert this to
if (rawobj->IsVMHeapObject() && (cid != kCodeCid) &&
HandleVMIsolateObject(rawobject)) {
  return true;
}

and avoid the whole writing_vm_isolate flag

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

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:322: RawArray* Symbols::UnifiedSymbolTable() {
ASSERT that this should only be called by the mutator thread and the background
compiler thread is stopped at this point.

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:368: }
This size computation seems to be needed only if FLAG_trace_compiler is  used,
can this code here be made conditional under that flag too.

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:417: }
Ditto comment about this size computation, probably only needed under a flag.

[rmacnak, 2016-05-05 20:40:57]

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

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:2286: if (rawobj->IsVMHeapObject() &&
HandleVMIsolateObject(rawobj)) {
On 2016/05/05 00:50:04, siva wrote:
> You could convert this to
> if (rawobj->IsVMHeapObject() && (cid != kCodeCid) &&
> HandleVMIsolateObject(rawobject)) {
>   return true;
> }
> 
> and avoid the whole writing_vm_isolate flag

It's not just kCodeCid but also kObjectPool (and whatever else a stub might
reference in the future). Filtering out kObjectPoolCid here is wrong because the
empty object pool should still be written as kEmptyObjectPool. The type isn't
the reason we should writing the object's body. We should be writing the body
because we are writing a vm isolate snapshot and the object isn't one with a
predefined id.
Alternatively we could prepopulate the forward list with all the objects with
predefined ids and remove HandleVMIsolateObject.

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

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:322: RawArray* Symbols::UnifiedSymbolTable() {
On 2016/05/05 00:50:05, siva wrote:
> ASSERT that this should only be called by the mutator thread and the
background
> compiler thread is stopped at this point.

Done.

https://codereview.chromium.org/1944213002/diff/40001/runtime/vm/symbols.cc#n...
runtime/vm/symbols.cc:368: }
On 2016/05/05 00:50:05, siva wrote:
> This size computation seems to be needed only if FLAG_trace_compiler is  used,
> can this code here be made conditional under that flag too.

Pushed to precompiler.cc

[rmacnak, 2016-05-06 23:08:59]

Description was changed from

==========
Towards taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Fix confused treatment of references to vm isolate objects as non-symbolic
when writing a precompiled regular isolate snapshot.

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

to

==========
Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
==========

[rmacnak, 2016-05-06 23:10:15]

Description was changed from

==========
Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
==========

to

==========
Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
 - Use the type of isolate we are writing instead of an object's current isolate
to decide if vm isolate objects are written symbolically.

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

[rmacnak, 2016-05-06 23:15:47]

Finished support for taking snapshots from a VM that was started from a
snapshot. PTAL.

./out/ReleaseX64/dart --use-blobs --snapshot=$PWD
--snapshot-kind=app-jit-after-run ~/hello.dart
./out/ReleaseX64/dart --use-blobs --run-app-snapshot=$PWD ignored.dart

[rmacnak, 2016-05-06 23:15:55]

Description was changed from

==========
Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
 - Use the type of isolate we are writing instead of an object's current isolate
to decide if vm isolate objects are written symbolically.

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

to

==========
Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
 - Use the type of isolate we are writing instead of an object's current isolate
to decide if vm isolate objects are written symbolically.

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

[siva, 2016-05-10 00:49:10]

lgtm

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

https://chromiumcodereview.appspot.com/1944213002/diff/60001/runtime/vm/preco...
runtime/vm/precompiler.cc:235: Symbols::GetStats(I, &symbols_before, &capacity);
Should these two calls to Symbols::GetStats be under
if (FLAG_trace_precompiler)

https://chromiumcodereview.appspot.com/1944213002/diff/100001/runtime/vm/snap...
File runtime/vm/snapshot.cc (right):

https://chromiumcodereview.appspot.com/1944213002/diff/100001/runtime/vm/snap...
runtime/vm/snapshot.cc:1926: // Write the object instead of a reference.
maybe make this comment a little more elaborate by stating that we are writing
the VM isolate snapshot here and hence we should not be writing out a VM object
id but the object itself.

https://chromiumcodereview.appspot.com/1944213002/diff/100001/runtime/vm/snap...
File runtime/vm/snapshot.h (right):

https://chromiumcodereview.appspot.com/1944213002/diff/100001/runtime/vm/snap...
runtime/vm/snapshot.h:992: bool writing_vm_isolate);
why not make this a parameter with an default initial value of false, that way
we would need this parameter to be passed in at only one callsite.

[rmacnak, 2016-05-10 01:45:02]

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

https://codereview.chromium.org/1944213002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:235: Symbols::GetStats(I, &symbols_before, &capacity);
On 2016/05/10 00:49:10, siva wrote:
> Should these two calls to Symbols::GetStats be under
> if (FLAG_trace_precompiler)

Done.

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

https://codereview.chromium.org/1944213002/diff/100001/runtime/vm/snapshot.cc...
runtime/vm/snapshot.cc:1926: // Write the object instead of a reference.
On 2016/05/10 00:49:10, siva wrote:
> maybe make this comment a little more elaborate by stating that we are writing
> the VM isolate snapshot here and hence we should not be writing out a VM
object
> id but the object itself.

Done.

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

https://codereview.chromium.org/1944213002/diff/100001/runtime/vm/snapshot.h#...
runtime/vm/snapshot.h:992: bool writing_vm_isolate);
On 2016/05/10 00:49:10, siva wrote:
> why not make this a parameter with an default initial value of false, that way
> we would need this parameter to be passed in at only one callsite.

Done.

[rmacnak, 2016-05-10 17:13:37]

Description was changed from

==========
Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
 - Use the type of isolate we are writing instead of an object's current isolate
to decide if vm isolate objects are written symbolically.

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

to

==========
Support for taking full snapshots from 'dart', not just 'dart_bootstrap'.

 - Don't create rival copies of the predefined symbols in isolates in
dart_bootstrap.
 - Don't do work to save these rival copies during symbol table compaction.
 - Create unified symbol table just before writing the vm isolate.
 - Create unified list of scripts to include in the vm isolate.
 - Ignore object ids of the old vm isolate when writing a new one.
 - Ensure token stream private keys are hashed.
 - Use the type of isolate we are writing instead of an object's current isolate
to decide if vm isolate objects are written symbolically.

BUG=
R=asiva@google.com

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

[rmacnak, 2016-05-10 17:13:39]

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