Making it possible to use V8 snapshots #21

wk8 · 2016-06-15T21:44:38Z

With proper unit tests

ignisf · 2016-06-15T22:01:02Z

Odd, it's failing on Linux

#
# Fatal error in .././src/snapshot/serialize.h, line 725
# Check failed: IsSane().
#

wk8 · 2016-06-15T22:06:21Z

@ignisf : dangit :( seems it failed on Travis too (for Linux as well).

How did you get that error message? I don't see that in Travis?

Trying on an Ubuntu VM.

ignisf · 2016-06-15T22:08:01Z

I'm no C++ programmer, but keep in mind that by default V8 on linux uses external snapshot data. We've worked around this issue by adding this flag to the V8 build process: https://github.com/cowboyd/libv8/blob/master/ext/libv8/builder.rb#L29-L30

That's about the only thing different I can think of between osx and linux. It could be something completely different.

ignisf · 2016-06-15T22:12:57Z

Relevant discussion: rubyjs/libv8#178 (comment)

ignisf · 2016-06-15T22:17:06Z

How did you get that error message? I don't see that in Travis?

ran bundle exec rake on my Arch Linux system.

ignisf · 2016-06-15T22:19:05Z

Hmm, I'm getting an /home/ignisf/Projects/mini_racer/lib/mini_racer.rb:93: [BUG] Illegal instruction at 0x007fbac6f91a09 though, and Travis build failure for 2.3.1 looks like a SEGFAULT...

wk8 · 2016-06-15T22:24:17Z

@ignisf : thanks for all that, looking into it shortly :)

ignisf · 2016-06-15T23:00:17Z

Ok, I think I'm onto something here. I tried this https://developers.google.com/v8/get_started#run-the-example example on my box, getting this:

[ignisf@clarity mini_racer]$  g++ -I/home/ignisf/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/libv8-5.1.281.59.1-x86_64-linux/vendor/v8/include test.cxx -o hello_world -Wl,--start-group /home/ignisf/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/gems/libv8-5.1.281.59.1-x86_64-linux/vendor/v8/out/x64.release/libv8_{base,libbase,external_snapshot,libplatform}.a -Wl,--end-group -lrt -ldl -pthread -std=c++0x
[ignisf@clarity mini_racer]$ ./hello_world 
Failed to open startup resource './natives_blob.bin'.
Failed to open startup resource './snapshot_blob.bin'.


#
# Fatal error in ../src/snapshot/natives-external.cc, line 122
# Check failed: holder_.
#

==== C stack trace ===============================

 1: 0x55d2e9f6e382
 2: 0x55d2e9f7399a
 3: 0x55d2e9aa552a
 4: 0x55d2e9aa5c34
 5: 0x55d2e9b21eeb
 6: 0x55d2e989c8ab
 7: 0x55d2e987a118
 8: __libc_start_main
 9: 0x55d2e9879f69
Illegal instruction (core dumped)
[ignisf@clarity mini_racer]$

ignisf · 2016-06-15T23:08:20Z

Copying those two files to the local directory allowed me to run the hello world. Here's a branch of libv8 for you to test with: https://github.com/cowboyd/libv8/commits/external-snapshot

ignisf · 2016-06-15T23:11:33Z

V8 requires its 'startup snapshot' to run. Copy the snapshot files to where your binary is stored:
cp out/x64.release/*.bin .

rofl

wk8 · 2016-06-15T23:15:43Z

@ignisf : thanks a lot for your help, much appreciated. Testing your libv8 branch :)

ignisf · 2016-06-15T23:19:49Z

np, hopefully you can get to the bottom of this

ignisf · 2016-06-15T23:25:37Z

just one more thing before I call it a night -- help me grasp the impact of this issue. Am I correct to think that the so called snapshot's purpose is to speed up context creation. And significant performance improvement is expected to be observed in cases which require the creation of many contexts?

wk8 · 2016-06-15T23:29:14Z

@ignisf : exactly. Our use case is: we have a base context that we could re-use across requests to do server-side pre-rendering. This would allow caching it once and for all

wk8 · 2016-06-15T23:29:35Z

And I didn't come up with the name "snapshot", V8 did ;-) http://v8project.blogspot.com/2015/09/custom-startup-snapshots.html

ignisf · 2016-06-16T00:18:13Z

https://github.com/v8/v8/blob/5.1.281.59/include/v8.h#L6618-L6636

wk8 · 2016-06-16T00:19:45Z

Thanks!

With proper unit tests

wk8 · 2016-06-16T22:37:44Z

@ignisf : fixed, was just me being dumb and having done too little C++ recently :-/

Thanks for your help though!

SamSaffron · 2016-06-17T05:42:52Z

A couple of questions:

Do we want to optionally allow to run the snapshot through WarmUpSnapshotDataBlob ? It should make it faster in theory
What happens if there is a parse error in the snapshot, is there any way to debug?

Is
StartupData startup_data = V8::CreateSnapshotDataBlob(RSTRING_PTR(str));
+

if (startup_data.data == NULL && startup_data.raw_size == 0) {

   rb_raise(rb_eSnapshotError, "Could not create snapshot, most likely the source is incorrect");

}

snapshot_info->data = startup_data.data;
snapshot_info->raw_size = startup_data.raw_size;

leaking a StartupData object somehow?

wk8 · 2016-06-17T15:53:30Z

@SamSaffron : thanks for your review.

WarmUpSnapshotDataBlob => good idea, happy to add that option, doing that today
debug if error in snapshot => not easy to do, and CreateSnapshotDataBlob does not provide any feedback in that case, except if they do that à la errno and failed to document it (see http://v8.paulfryzel.com/docs/master/classv8_1_1_v8.html#a723f993d0fb7f408576c7143bb8dfa6e); I've found the best way to debug faulty snapshots is to try & eval them in an empty context, where V8 can then give more explicit error messages (happy to add that to the MiniRacer::SnapshotError exception's message if you want)
leaking a StartupData object somehow => I don't think so, StartupData objects only contain raw data as a char* and its size as an int (see https://github.com/v8/v8/blob/1298aef51fc0e7569e1b9c5905ad8d29a2903246/include/v8.h#L6371-6375) so if the char* is NULL, as it is when the exception is thrown, then there's nothing to be leaked

wk8 · 2016-06-17T16:30:44Z

@SamSaffron : snapshots warm up added (edeaa9d)

ignisf · 2016-06-17T16:40:42Z

ext/mini_racer_extension/mini_racer_extension.cc

+    StartupData warm_startup_data = V8::WarmUpSnapshotDataBlob(cold_startup_data, RSTRING_PTR(str));
+
+    if (warm_startup_data.data == NULL && warm_startup_data.raw_size == 0) {
+        rb_raise(rb_eSnapshotError, "Could not warp up snapshot, most likely the source is incorrect");


Typo? Or am I missing the definition of the awesome sounding term "warping up"? :)

Er, thanks, fixing ;)

Which maps to V8's `V8::WarmUpSnapshotDataBlob` (see http://v8.paulfryzel.com/docs/master/classv8_1_1_v8.html#abee465cc67755cfaacf0297c3093fb47) Added unit tests on it.

LtSquigs · 2016-06-17T17:02:20Z

Just a quick note on WarmUpSnapshotDataBlob, it appears to be mostly just to allow you to call functions to get them to compile (as noted in the comments of the function https://github.com/v8/v8/blob/master/src/api.cc#L520)

This is because functions are lazily compiled (electron/electron#169 (comment)), the solution in that thread from the creator of snapshots was just to call the functions but wrap them in try/catch to force them to compile. WarmUpSnapshotDataBlob just seems to be a more formal way to solve that particular issue.

SamSaffron · 2016-06-19T23:37:41Z

OK I will merge this in, @wk8 since we are getting all fancy here one area that is very weak atm is memory management and visibility, I would love to get some stuff in here.

#18

at a minimum visibility on what is currently in use by v8, but more predictable v8 disposal is also quite important (and especially for the snapshot usage, cause you want to dispose the v8 resources asap once you are done)

wk8 · 2016-06-20T19:42:05Z

Thanks for merging!

wk8 force-pushed the wk8/snapshots branch 2 times, most recently from 7f26c02 to 1b4ed72 Compare June 15, 2016 23:35

wk8 force-pushed the wk8/snapshots branch 3 times, most recently from 6f9af1e to 1739ffc Compare June 16, 2016 22:23

Making it possible to use V8 snapshots

6063b13

With proper unit tests

wk8 force-pushed the wk8/snapshots branch from 1739ffc to 6063b13 Compare June 16, 2016 22:24

Merge remote-tracking branch 'origin/master' into wk8/snapshots

764a084

ignisf reviewed Jun 17, 2016
View reviewed changes

Adding a warmup method on snapshots

edeaa9d

Which maps to V8's `V8::WarmUpSnapshotDataBlob` (see http://v8.paulfryzel.com/docs/master/classv8_1_1_v8.html#abee465cc67755cfaacf0297c3093fb47) Added unit tests on it.

wk8 force-pushed the wk8/snapshots branch from 5cebde1 to edeaa9d Compare June 17, 2016 16:42

SamSaffron merged commit bddbdcd into rubyjs:master Jun 19, 2016

Making it possible to use V8 snapshots #21

Making it possible to use V8 snapshots #21

Uh oh!

Conversation

wk8 commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016

Uh oh!

wk8 commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ignisf commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wk8 commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016

Uh oh!

wk8 commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016

Uh oh!

ignisf commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wk8 commented Jun 15, 2016

Uh oh!

wk8 commented Jun 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ignisf commented Jun 16, 2016

Uh oh!

wk8 commented Jun 16, 2016

Uh oh!

wk8 commented Jun 16, 2016

Uh oh!

SamSaffron commented Jun 17, 2016

Uh oh!

wk8 commented Jun 17, 2016

Uh oh!

wk8 commented Jun 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ignisf Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

wk8 Jun 17, 2016

Choose a reason for hiding this comment

Uh oh!

LtSquigs commented Jun 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamSaffron commented Jun 19, 2016

Uh oh!

wk8 commented Jun 20, 2016

Uh oh!

Uh oh!

ignisf commented Jun 15, 2016 •

edited

Loading

ignisf commented Jun 15, 2016 •

edited

Loading

ignisf commented Jun 15, 2016 •

edited

Loading

wk8 commented Jun 15, 2016 •

edited

Loading

wk8 commented Jun 17, 2016 •

edited

Loading

LtSquigs commented Jun 17, 2016 •

edited

Loading