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

Add native support for loading ES modules #1440

Merged
merged 7 commits into from
Jan 3, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ environment:
RUST_DIR: $(USERPROFILE)\rust
CARGO_HOME: $(RUST_DIR)\cargo
RUSTUP_HOME: $(RUST_DIR)\rustup
RUST_BACKTRACE: 1
RUST_BACKTRACE: full
RUSTC_WRAPPER: sccache
SCCACHE_BUCKET: deno-sccache
AWS_ACCESS_KEY_ID: AKIAIVRN52PLDBP55LBQ
Expand Down Expand Up @@ -238,8 +238,8 @@ cache:
- $(APPVEYOR_BUILD_FOLDER)\third_party
# Cache file mtimes in the main git repo, also to enable incremental builds.
- $(MTIME_CACHE_DB)
# Build incrementally.
- $(DENO_BUILD_PATH)
# TODO Incremental build disabled because there are some bugs.
#- $(DENO_BUILD_PATH)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it super slow now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


init:
# Load utility functions
Expand Down
8 changes: 4 additions & 4 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ env:
global:
- CARGO_HOME=$TRAVIS_BUILD_DIR/third_party/rust_crates/
- RUSTUP_HOME=$HOME/.rustup/
- RUST_BACKTRACE=1
- RUST_BACKTRACE=full
- CARGO_TARGET_DIR=$HOME/target
- PATH=$TRAVIS_BUILD_DIR/third_party/llvm-build/Release+Asserts/bin:$CARGO_HOME/bin:$PATH
- RUSTC_WRAPPER=sccache
Expand Down Expand Up @@ -64,7 +64,7 @@ before_script:
script:
- ./tools/lint.py
- ./tools/test_format.py
- ./tools/build.py -C target/release -j2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is based on this #1444 (explanation there)

- ./tools/build.py -C target/release
- DENO_BUILD_MODE=release ./tools/test.py

jobs:
Expand Down Expand Up @@ -106,7 +106,7 @@ jobs:
- name: "cargo release linux"
os: linux
script:
- cargo build -vv --release -j2
- cargo build -vv --release

# LSAN: We are in the process of getting a completely clean LSAN build,
# but it will take some work. So for now we just run a subset of the
Expand All @@ -121,6 +121,6 @@ jobs:
# Call gn gen again to make sure new args are recognized.
- third_party/depot_tools/gn gen target/debug
- export ASAN_OPTIONS=detect_leaks=1
- ./tools/build.py test_cc -j2
- ./tools/build.py test_cc
- ./target/debug/test_cc

21 changes: 21 additions & 0 deletions libdeno/api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ deno::DenoIsolate* unwrap(Deno* d_) {
deno_buf deno_get_snapshot(Deno* d_) {
auto* d = unwrap(d_);
CHECK_NE(d->snapshot_creator_, nullptr);
CHECK(d->resolve_module_.IsEmpty());
d->ClearModules();
d->context_.Reset();

auto blob = d->snapshot_creator_->CreateBlob(
v8::SnapshotCreator::FunctionCodeHandling::kClear);
return {nullptr, 0, reinterpret_cast<uint8_t*>(const_cast<char*>(blob.data)),
Expand Down Expand Up @@ -123,6 +126,19 @@ int deno_execute(Deno* d_, void* user_data, const char* js_filename,
return deno::Execute(context, js_filename, js_source) ? 1 : 0;
}

int deno_execute_mod(Deno* d_, void* user_data, const char* js_filename,
const char* js_source) {
auto* d = unwrap(d_);
deno::UserDataScope user_data_scope(d, user_data);
auto* isolate = d->isolate_;
v8::Locker locker(isolate);
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);
auto context = d->context_.Get(d->isolate_);
CHECK(!context.IsEmpty());
return deno::ExecuteMod(context, js_filename, js_source) ? 1 : 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this return value? Success/error? In c++ code I would expect to use bool, or alternatively you can do common c way of 0 (ok) / -1 (fail).

Copy link
Member Author

@ry ry Jan 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See docs in deno.h - this is to match the behavior of deno_execute 1 success / 0 fail.

(Yes I agree it should be the opposite, but let's do that in a separate PR.)

}

int deno_respond(Deno* d_, void* user_data, int32_t req_id, deno_buf buf) {
auto* d = unwrap(d_);
if (d->current_args_ != nullptr) {
Expand Down Expand Up @@ -193,4 +209,9 @@ void deno_terminate_execution(Deno* d_) {
deno::DenoIsolate* d = reinterpret_cast<deno::DenoIsolate*>(d_);
d->isolate_->TerminateExecution();
}

void deno_resolve_ok(Deno* d_, const char* filename, const char* source) {
deno::DenoIsolate* d = reinterpret_cast<deno::DenoIsolate*>(d_);
d->ResolveOk(filename, source);
}
}
170 changes: 150 additions & 20 deletions libdeno/binding.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,6 @@ const char* ToCString(const v8::String::Utf8Value& value) {
return *value ? *value : "<string conversion failed>";
}

static inline v8::Local<v8::String> v8_str(const char* x) {
return v8::String::NewFromUtf8(v8::Isolate::GetCurrent(), x,
v8::NewStringType::kNormal)
.ToLocalChecked();
}

std::string EncodeExceptionAsJSON(v8::Local<v8::Context> context,
v8::Local<v8::Value> exception) {
auto* isolate = context->GetIsolate();
Expand Down Expand Up @@ -291,7 +285,7 @@ void Send(const v8::FunctionCallbackInfo<v8::Value>& args) {
DCHECK_EQ(d->isolate_, isolate);

v8::Locker locker(d->isolate_);
v8::EscapableHandleScope handle_scope(isolate);
v8::HandleScope handle_scope(isolate);

CHECK_EQ(d->current_args_, nullptr); // libdeno.send re-entry forbidden.
int32_t req_id = d->next_req_id_++;
Expand Down Expand Up @@ -349,34 +343,146 @@ void Shared(v8::Local<v8::Name> property,
info.GetReturnValue().Set(ab);
}

bool ExecuteV8StringSource(v8::Local<v8::Context> context,
const char* js_filename,
v8::Local<v8::String> source) {
v8::ScriptOrigin ModuleOrigin(v8::Local<v8::Value> resource_name,
v8::Isolate* isolate) {
return v8::ScriptOrigin(resource_name, v8::Local<v8::Integer>(),
v8::Local<v8::Integer>(), v8::Local<v8::Boolean>(),
v8::Local<v8::Integer>(), v8::Local<v8::Value>(),
v8::Local<v8::Boolean>(), v8::Local<v8::Boolean>(),
v8::True(isolate));
}

void DenoIsolate::ClearModules() {
for (auto it = module_map_.begin(); it != module_map_.end(); it++) {
it->second.Reset();
}
module_map_.clear();
module_filename_map_.clear();
}

void DenoIsolate::RegisterModule(const char* filename,
v8::Local<v8::Module> module) {
int id = module->GetIdentityHash();

// v8.h says that identity hash is not necessarily unique. It seems it's quite
// unique enough for the purposes of O(1000) modules, so we use it as a
// hashmap key here. The following check is to detect collisions.
CHECK_EQ(0, module_filename_map_.count(id));

module_filename_map_[id] = filename;
module_map_.emplace(std::piecewise_construct, std::make_tuple(filename),
std::make_tuple(isolate_, module));
}

v8::MaybeLocal<v8::Module> CompileModule(v8::Local<v8::Context> context,
const char* js_filename,
v8::Local<v8::String> source_text) {
auto* isolate = context->GetIsolate();

v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);
v8::EscapableHandleScope handle_scope(isolate);
v8::Context::Scope context_scope(context);

auto origin = ModuleOrigin(v8_str(js_filename, true), isolate);
v8::ScriptCompiler::Source source(source_text, origin);

auto maybe_module = v8::ScriptCompiler::CompileModule(isolate, &source);

if (!maybe_module.IsEmpty()) {
auto module = maybe_module.ToLocalChecked();
CHECK_EQ(v8::Module::kUninstantiated, module->GetStatus());
DenoIsolate* d = FromIsolate(isolate);
d->RegisterModule(js_filename, module);
}

return handle_scope.EscapeMaybe(maybe_module);
}

v8::MaybeLocal<v8::Module> ResolveCallback(v8::Local<v8::Context> context,
v8::Local<v8::String> specifier,
v8::Local<v8::Module> referrer) {
auto* isolate = context->GetIsolate();
DenoIsolate* d = FromIsolate(isolate);

v8::Isolate::Scope isolate_scope(isolate);
v8::EscapableHandleScope handle_scope(isolate);
v8::Context::Scope context_scope(context);

v8::TryCatch try_catch(isolate);
int ref_id = referrer->GetIdentityHash();
std::string referrer_filename = d->module_filename_map_[ref_id];

auto name = v8_str(js_filename);
v8::String::Utf8Value specifier_(isolate, specifier);
const char* specifier_c = ToCString(specifier_);

v8::ScriptOrigin origin(name);
CHECK_NE(d->resolve_cb_, nullptr);
d->resolve_cb_(d->user_data_, specifier_c, referrer_filename.c_str());

auto script = v8::Script::Compile(context, source, &origin);
if (d->resolve_module_.IsEmpty()) {
// Resolution Error.
isolate->ThrowException(v8_str("module resolution error"));
return v8::MaybeLocal<v8::Module>();
} else {
auto module = d->resolve_module_.Get(isolate);
d->resolve_module_.Reset();
return handle_scope.Escape(module);
}
}

if (script.IsEmpty()) {
void DenoIsolate::ResolveOk(const char* filename, const char* source) {
CHECK(resolve_module_.IsEmpty());
auto count = module_map_.count(filename);
if (count == 1) {
auto module = module_map_[filename].Get(isolate_);
resolve_module_.Reset(isolate_, module);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that there are chances where we might fetch a source multiple times, but discard all the fetched source code and only using the first fetched version?

(As a side question, does V8 InstantiateModule guarantees that the same module would not be running in the same context twice?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe (tho I'm not sure, I haven't tested it) that the module_map_ will properly prevent that from happening. The resolve_module_.Reset() here is only to pass around a reference during resolution.

} else {
CHECK_EQ(count, 0);
v8::HandleScope handle_scope(isolate_);
auto context = context_.Get(isolate_);
v8::TryCatch try_catch(isolate_);
auto maybe_module = CompileModule(context, filename, v8_str(source));
if (maybe_module.IsEmpty()) {
DCHECK(try_catch.HasCaught());
HandleException(context, try_catch.Exception());
} else {
auto module = maybe_module.ToLocalChecked();
resolve_module_.Reset(isolate_, module);
}
}
}

bool ExecuteMod(v8::Local<v8::Context> context, const char* js_filename,
const char* js_source) {
auto* isolate = context->GetIsolate();
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);
v8::Context::Scope context_scope(context);

auto source = v8_str(js_source, true);

v8::TryCatch try_catch(isolate);

auto maybe_module = CompileModule(context, js_filename, source);

if (maybe_module.IsEmpty()) {
DCHECK(try_catch.HasCaught());
HandleException(context, try_catch.Exception());
return false;
}
DCHECK(!try_catch.HasCaught());

auto result = script.ToLocalChecked()->Run(context);
auto module = maybe_module.ToLocalChecked();
auto maybe_ok = module->InstantiateModule(context, ResolveCallback);
if (maybe_ok.IsNothing()) {
return false;
}

CHECK_EQ(v8::Module::kInstantiated, module->GetStatus());
auto result = module->Evaluate(context);

if (result.IsEmpty()) {
DCHECK(try_catch.HasCaught());
HandleException(context, try_catch.Exception());
CHECK_EQ(v8::Module::kErrored, module->GetStatus());
HandleException(context, module->GetException());
return false;
}

Expand All @@ -388,8 +494,32 @@ bool Execute(v8::Local<v8::Context> context, const char* js_filename,
auto* isolate = context->GetIsolate();
v8::Isolate::Scope isolate_scope(isolate);
v8::HandleScope handle_scope(isolate);
auto source = v8_str(js_source);
return ExecuteV8StringSource(context, js_filename, source);
v8::Context::Scope context_scope(context);

auto source = v8_str(js_source, true);
auto name = v8_str(js_filename, true);

v8::TryCatch try_catch(isolate);

v8::ScriptOrigin origin(name);

auto script = v8::Script::Compile(context, source, &origin);

if (script.IsEmpty()) {
DCHECK(try_catch.HasCaught());
HandleException(context, try_catch.Exception());
return false;
}

auto result = script.ToLocalChecked()->Run(context);

if (result.IsEmpty()) {
DCHECK(try_catch.HasCaught());
HandleException(context, try_catch.Exception());
return false;
}

return true;
}

void InitializeContext(v8::Isolate* isolate, v8::Local<v8::Context> context) {
Expand Down
31 changes: 25 additions & 6 deletions libdeno/deno.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,26 @@ typedef struct deno_s Deno;
typedef void (*deno_recv_cb)(void* user_data, int32_t req_id,
deno_buf control_buf, deno_buf data_buf);

// A callback to implement ES Module imports. User must call deno_resolve_ok()
// at most once during deno_resolve_cb. If deno_resolve_ok() is not called, the
// specifier is considered invalid and will issue an error in JS. The reason
// deno_resolve_cb does not return deno_module is to avoid unnecessary heap
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not return deno_module is to avoid unnecessary heap allocations

Did it seem very expensive? I doubt this is even in the vicinity of a hot path.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not so much about performance, but rather about keeping memory management simple. I don't want to have to think about allocating something in Rust and then passing ownership to libdeno

// allocations.
typedef void (*deno_resolve_cb)(void* user_data, const char* specifier,
const char* referrer);

void deno_resolve_ok(Deno* d, const char* filename, const char* source);

void deno_init();
const char* deno_v8_version();
void deno_set_v8_flags(int* argc, char** argv);

typedef struct {
int will_snapshot; // Default 0. If calling deno_get_snapshot 1.
deno_buf load_snapshot; // Optionally: A deno_buf from deno_get_snapshot.
deno_buf shared; // Shared buffer to be mapped to libdeno.shared
deno_recv_cb recv_cb; // Maps to libdeno.send() calls.
int will_snapshot; // Default 0. If calling deno_get_snapshot 1.
deno_buf load_snapshot; // Optionally: A deno_buf from deno_get_snapshot.
deno_buf shared; // Shared buffer to be mapped to libdeno.shared
deno_recv_cb recv_cb; // Maps to libdeno.send() calls.
deno_resolve_cb resolve_cb; // Each import calls this.
} deno_config;

// Create a new deno isolate.
Expand All @@ -47,9 +58,10 @@ deno_buf deno_get_snapshot(Deno* d);

void deno_delete(Deno* d);

// Returns false on error.
// Compile and execute a traditional JavaScript script that does not use
// module import statements.
// Return value: 0 = fail, 1 = success
// Get error text with deno_last_exception().
// 0 = fail, 1 = success
//
// TODO change return value to be const char*. On success the return
// value is nullptr, on failure it is the JSON exception text that
Expand All @@ -59,6 +71,13 @@ void deno_delete(Deno* d);
int deno_execute(Deno* d, void* user_data, const char* js_filename,
const char* js_source);

// Compile and execute an ES module. Caller must have provided a deno_resolve_cb
// when instantiating the Deno object.
// Return value: 0 = fail, 1 = success
// Get error text with deno_last_exception().
int deno_execute_mod(Deno* d, void* user_data, const char* js_filename,
const char* js_source);

// deno_respond sends up to one message back for every deno_recv_cb made.
//
// If this is called during deno_recv_cb, the issuing libdeno.send() in
Expand Down
Loading