Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot run a snapshot with --preview-dart-2 #32911

Closed
natebosch opened this issue Apr 17, 2018 · 22 comments
Closed

Cannot run a snapshot with --preview-dart-2 #32911

natebosch opened this issue Apr 17, 2018 · 22 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone

Comments

@natebosch
Copy link
Member

If I create a snapshot without --preview-dart-2 and try to run it with --preview-dart-2 I get some long stack traces starting with:

Crash when formatting: [Encoding] Unable to decode bytes as UTF-8.
RangeError (index): Invalid value: Not in range 0..5792, inclusive: 5813
#0      _StringBase.[] (dart:core/runtime/libstring_patch.dart:233:55)
#1      Source.getTextLine (package:kernel/ast.dart:5695:42)
#2      getSourceLine (package:front_end/src/fasta/messages.dart:27:9)

If I create a snapshot with --preview-dart-2 and try to run it with --preview-dart-2 I get a bunch of garbage output that make it look like it's trying to parser the binary file as dart text source then a stack trace.

If I create a snapshot with --preview-dart-2 and try to run it without any VM arguments I get a sensible error:

Snapshot not compatible with the current VM configuration: the snapshot requires 'release strong' but the VM has 'release no-strong'
@kevmoo kevmoo added the area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. label Apr 17, 2018
@vsmenon vsmenon added this to the Dart2Beta4 milestone Apr 19, 2018
@vsmenon vsmenon added the P1 A high priority bug; for example, a single project is unusable or has many test failures label Apr 19, 2018
@vsmenon
Copy link
Member

vsmenon commented Apr 19, 2018

Do we need this fixed to unblock #32188 ?

@vsmenon
Copy link
Member

vsmenon commented Apr 19, 2018

@a-siva @mraleph

dart-bot pushed a commit that referenced this issue Apr 19, 2018
Script snapshots add nothing over kernel files.

Bug: #32911
Change-Id: If7a58ce8ec5305462e9bf449d02394c87579c9a4
Reviewed-on: https://dart-review.googlesource.com/51801
Reviewed-by: Siva Annamalai <asiva@google.com>
Commit-Queue: Ryan Macnak <rmacnak@google.com>
@natebosch
Copy link
Member Author

Do we need this fixed to unblock #32188 ?

We can make some progress by running from source always for Dart 2 mode but we'll be paying a performance cost.

@keertip
Copy link
Contributor

keertip commented Apr 19, 2018

@natebosch, in Dart 2 appjit snapshots is what works, no script snapshots. Did you try an appjit?

@natebosch
Copy link
Member Author

No we have not tried appjit snapshots. This will require some work in pub, I'm not sure how significant.

Is there reason we aren't supporting non-jit snapshots?

@keertip
Copy link
Contributor

keertip commented Apr 19, 2018

In dart 2, the kernel file is the equivalent of script snapshot. You can generate kernel files for the vm using the gen_kernel tool and then pass this to the VM instead of source.

@natebosch
Copy link
Member Author

I can't find gen_kernel in the dev.48 sdk bin directory - where are we shipping it?

@keertip
Copy link
Contributor

keertip commented Apr 19, 2018

@a-siva @alexmarkov, we are not shipping the gen_kernel tool in the sdk build. Could you add this?

@dgrove
Copy link
Contributor

dgrove commented Apr 20, 2018

cc @mit-mit we should think about whether gen_kernel is the right name for this tool.

@jakobr-google
Copy link
Contributor

So, um, why do we need a different tool? Why doesn't dart --preview-dart-2 --snapshot=<snapshot-file> <dart-file> produce a snapshot in the right format?

@jensjoha
Copy link
Contributor

Script snapshotting through kernel _used_to work...

Now though I get this:

$ pkg/vm/tool/dart2 --snapshot=foo.snapshot hello.dart

$ ls -lha foo.snapshot
-rw-r----- 1 jensj primarygroup 847 Apr 20 12:22 foo.snapshot

$ out/ReleaseX64/dart foo.snapshot
Snapshot not compatible with the current VM configuration: the snapshot requires 'release strong' but the VM has 'release no-strong'

$ out/ReleaseX64/dart --strong foo.snapshot                            
Snapshot not compatible with the current VM configuration: the snapshot requires 'release no-strong' but the VM has 'release strong'

@mit-mit
Copy link
Member

mit-mit commented Apr 20, 2018

Big +1 for making it work again, and not introducing yet another tool.

@jensjoha
Copy link
Contributor

jensjoha commented Apr 20, 2018

Interestingly this still works:

$ pkg/front_end/tool/fasta compile hello.dart

$ out/ReleaseX64/dart --snapshot=foo.snapshot hello.dart.dill

$ ls -lha foo.snapshot
-rw-r----- 1 jensj primarygroup 452K Apr 20 15:39 foo.snapshot

$ out/ReleaseX64/dart foo.snapshot
Hello, world!

Updated last command...

@vsmenon
Copy link
Member

vsmenon commented Apr 20, 2018

@a-siva - assigning to you. We need this to create and run Dart 2 snapshots for pub, test, etc.

@mraleph
Copy link
Member

mraleph commented Apr 20, 2018

@natebosch do we need this to work only when building SDK or do we need this to work from SDK itself too?

@natebosch
Copy link
Member Author

We need this to work externally to the SDK.

pub builds snapshots of binaries to run them, and if we're going to have to pay a ~10 second penalty for running from source we'll really want to keep doing that so we only pay the parse cost once.

Similarly in build_runner we currently rely on starting an Isolate on a dart source file we wrote. If there is a startup performance penalty we need to start using a snapshot there too.

@jensjoha
Copy link
Contributor

jensjoha commented Apr 23, 2018

Disclaimer: I'm not suggesting we do this exactly as such, but the following hack (or proof of concept or whatnot) at least works on a hello world script:

  1. Tried on top of 1f7fa3e
  2. Revert da14cbd.
  3. Apply the following patch:
diff --git a/runtime/bin/dfe.cc b/runtime/bin/dfe.cc
index 6bccb8de40..c6387b8e1b 100644
--- a/runtime/bin/dfe.cc
+++ b/runtime/bin/dfe.cc
@@ -230,6 +230,30 @@ void* DFE::CompileAndReadScript(const char* script_uri,
       strong ? platform_strong_dill : platform_dill;
   intptr_t platform_binary_size =
       strong ? platform_strong_dill_size : platform_dill_size;
+
+  {
+    const uint8_t* buffer = NULL;
+    intptr_t buffer_length = -1;
+    const uint8_t** kernel_ir = &buffer;
+    intptr_t* kernel_ir_size = &buffer_length;
+
+    *kernel_ir = NULL;
+    *kernel_ir_size = -1;
+    void* script_file = DartUtils::OpenFileUri(script_uri, false);
+    if (script_file != NULL) {
+      const uint8_t* buffer = NULL;
+      DartUtils::ReadFile(&buffer, kernel_ir_size, script_file);
+      DartUtils::CloseFile(script_file);
+      if (*kernel_ir_size != 0 && buffer != NULL) {
+        if (DartUtils::SniffForMagicNumber(buffer, *kernel_ir_size) ==
+            DartUtils::kSnapshotMagicNumber) {
+          Dart_EnterScope();
+          return Dart_LoadScriptFromSnapshot(buffer, *kernel_ir_size);
+        }
+      }
+    }
+  }
+
   Dart_KernelCompilationResult result = Dart_CompileToKernel(
       sanitized_uri, platform_binary, platform_binary_size);
   switch (result.status) {
diff --git a/runtime/vm/dart_api_impl.cc b/runtime/vm/dart_api_impl.cc
index eff8af86ba..b80c021237 100644
--- a/runtime/vm/dart_api_impl.cc
+++ b/runtime/vm/dart_api_impl.cc
@@ -5129,9 +5129,7 @@ DART_EXPORT Dart_Handle Dart_LoadScript(Dart_Handle url,
   }
   Library& library = Library::Handle(Z, I->object_store()->root_library());
   if (!library.IsNull()) {
-    const String& library_url = String::Handle(Z, library.url());
-    return Api::NewError("%s: A script has already been loaded from '%s'.",
-                         CURRENT_FUNC, library_url.ToCString());
+    return Api::NewHandle(T, library.raw());
   }
   if (line_offset < 0) {
     return Api::NewError("%s: argument 'line_offset' must be positive number",
  1. Try this:
$ time pkg/vm/tool/dart2 hello.dart
Hello, world!

real    0m0.401s
user    0m0.552s
sys     0m0.112s

$ rm foo.snapshot

$ time pkg/vm/tool/dart2 --snapshot=foo.snapshot hello.dart

real    0m0.512s
user    0m0.744s
sys     0m0.140s

$ ls -la foo.snapshot
-rw-r----- 1 jensj primarygroup 847 Apr 23 10:43 foo.snapshot

$ time pkg/vm/tool/dart2 foo.snapshot
Hello, world!

real    0m0.193s
user    0m0.276s
sys     0m0.080s

Again: I'm not saying we should do it like this, but maybe this can give someone else a starting point.

Update: I just tried on dart2js, and that doesn't work with the above. It appears that many dart: libraries are included in the snapshot and it thus hits ASSERT(Library::LookupLibrary(thread, lib_url) == Library::null()); when calling Library::Register.

@a-siva
Copy link
Contributor

a-siva commented Apr 26, 2018

CL https://dart-review.googlesource.com/c/sdk/+/52740 out for review

sivalinuxmach[sdk]>out/ReleaseX64/dart --preview-dart-2 --snapshot=/tmp/hello.snapshot /tmp/hello.dart
sivalinuxmach[sdk]>out/ReleaseX64/dart /tmp/hello.snapshot
Hello World
sivalinuxmach[sdk]>

@dgrove
Copy link
Contributor

dgrove commented Apr 27, 2018

Any update on this, @a-siva ?

@dgrove
Copy link
Contributor

dgrove commented May 3, 2018

CL https://dart-review.googlesource.com/c/sdk/+/52740 has landed. Can this be closed?

@a-siva
Copy link
Contributor

a-siva commented May 3, 2018

Yes the CL has landed and stuck, closing issue.

@a-siva a-siva closed this as completed May 3, 2018
@kevmoo
Copy link
Member

kevmoo commented May 3, 2018

4b1180a – landed in -dev.53

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

10 participants