Add libdeno_nosnapshot source_set #359
Conversation
There was a problem hiding this comment.
Awesome!
I haven't tested this yet, but I do think this should be a separate target. The reason is that there are situations where we would want to debug the snapshot build.
cc @mreinstein
src/from_filesystem.cc
Outdated
| } | ||
|
|
||
| Deno* NewFromFileSystem(void* data, deno_recv_cb cb) { | ||
| // TODO(f-a-a) reference this dynamically somehow? |
There was a problem hiding this comment.
One idea is to use argv[0] to locate the deno executable and try to reference the bundle relatively from there. That said, I think this is fine for a first pass.
You might try something like this in the BUILD.gn to define where the bundle file is more robustly
bl = rebase_path(get_target_outputs(":bundle")[0], root_build_dir)
define = [ "BUNDLE_LOCATION=" + bl ]
|
@ry PTAL! What changed:
|
BUILD.gn
Outdated
| deps = [ ":libdeno" ] | ||
| } | ||
|
|
||
| rust_executable("deno_nosnapshot") { |
There was a problem hiding this comment.
Can you add a deno_cc_nosnapshot too. At the moment we're using deno_cc target because rust support for flatbuffers is not complete.
Also, before the deno_nosnapshot and deno_cc_nosnapshot targets, please add some comment about their purpose. Example comment (please edit)
# This target is for fast incremental development. When modifying the javascript runtime this target will not
# go through the extra process of building a snapshot and instead load the bundle from disk.
src/from_filesystem.cc
Outdated
|
|
||
| Deno* NewFromFileSystem(void* data, deno_recv_cb cb) { | ||
| std::string js_source; | ||
| CHECK(deno::ReadFileToString(BUNDLE_LOCATION, &js_source)); |
There was a problem hiding this comment.
For debugging purposes please add this before the ReadFileToString
printf("Reading javascript runtime bundle from " BUNDLE_LOCATION "\n");
BUILD.gn
Outdated
| ] | ||
| configs += [ ":deno_config" ] | ||
| bt = get_target_outputs(":bundle") | ||
| bl = rebase_path(bt[0], root_build_dir) |
There was a problem hiding this comment.
If you use rebase_path(bt[0]) instead, it will set the BUNDLE_LOCATION to be an absolute path, which I think is better.
Also please use more descriptive variable names here bundle_outputs and bundle_location
src/from_filesystem.cc
Outdated
|
|
||
| std::vector<InternalFieldData*> deserialized_data; | ||
|
|
||
| void DeserializeInternalFields(v8::Local<v8::Object> holder, int index, |
There was a problem hiding this comment.
This is unnecessary and can be removed. There's no snapshot.
src/from_filesystem.cc
Outdated
| v8::Context::New(isolate, nullptr, v8::MaybeLocal<v8::ObjectTemplate>(), | ||
| v8::MaybeLocal<v8::Value>(), | ||
| v8::DeserializeInternalFieldsCallback( | ||
| DeserializeInternalFields, nullptr)); |
There was a problem hiding this comment.
Remove all but the first argument to Context::New
src/handlers.rs
Outdated
| cmd_id, | ||
| module_specifier, | ||
| containing_file | ||
| cmd_id, module_specifier, containing_file |
There was a problem hiding this comment.
Unrelated change (it should have been formatted elsewhere, but it would be good if you left it out)
There was a problem hiding this comment.
gotcha, sorry about that. I'll leave this in for now yeah? (I could drop the commit though)
Edit: I dropped the commit
BUILD.gn
Outdated
| configs += [ ":deno_config" ] | ||
| bundle_outputs = get_target_outputs(":bundle") | ||
| bundle_location = rebase_path(bundle_outputs[0]) | ||
| defines = [ "BUNDLE_LOCATION=\"${bundle_location}\"" ] |
There was a problem hiding this comment.
I think no curly braces around bundle_location
"BUNDLE_LOCATION=\"$bundle_location\""
src/from_filesystem.cc
Outdated
| v8::Isolate::CreateParams params; | ||
| params.array_buffer_allocator = | ||
| v8::ArrayBuffer::Allocator::NewDefaultAllocator(); | ||
| params.external_references = external_references; |
There was a problem hiding this comment.
This line is unnecessary (has to do with snapshots. sorry for not catching it earlier)
This PR attempts to solve #311.
I wouldn't expect for this PR to be merged right away, but this is a proof of concept I've been experimenting with after taking a stab at it.
This PR introduces a new
src/from_filesystem.ccthat is akin tosrc/from_snapshot.ccexcept that instead of initializing the v8 context withStartupBlob_snapshot(), it loads js from filesystem using theInitializeContextexposed ininternal.h.It doesn't introduce a new target as @ry suggested in the issue. Instead, ais_debugcheck inBUILD.gnwill conditionally buildlibdeno_nosnapshotif it is true.It does now!
Benchmarks when adding new
deno.printtomain.tsfile:master build log
deno_nosnapshot build log
Footnote
Currently
js_filenamereference is hard coded to./out/Debug/gen/bundle/main.js. I'm sure there is a smarter way of obtaining the bundle path.I would gladly make the necessary changes to have this PR merged if the concept is sound, otherwise I hope this could be an inspiration for others to work on it!