From 7778c035a0a3f9bbb988eb0298a026d80f423e03 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 14 Nov 2018 22:38:12 +0800 Subject: [PATCH] src: use STL containers instead of v8 values for static module data Instead of putting the source code and the cache in v8::Objects, put them in per-process std::maps. This has the following benefits: - It's slightly lighter in weight compared to storing things on the v8 heap. Also it may be slightly faster since the preivous v8::Object is already in dictionary mode - though the difference is very small given the number of native modules is limited. - The source and code cache generation templates are now much simpler since they just initialize static arrays and manipulate STL constructs. - The static native module data can be accessed independently of any Environment or Isolate, and it's easy to look them up from the C++'s side. - It's now impossible to mutate the source code used to compile native modules from the JS land since it's completely separate from the v8 heap. We can still get the constant strings from process.binding('natives') but that's all. A few drive-by fixes: - Remove DecorateErrorStack in LookupAndCompile - We don't need to capture the exception to decorate when we encounter errors during native module compilation, as those errors should be syntax errors and v8 is able to decorate them well. We use CompileFunctionInContext so there is no need to worry about wrappers either. - The code cache could be rejected when node is started with v8 flags. Instead of aborting in that case, simply keep a record in the native_module_without_cache set. - Refactor js2c.py a bit, reduce code duplication and inline Render() to make the one-byte/two-byte special treatment easier to read. PR-URL: https://github.com/nodejs/node/pull/24384 Fixes: https://github.com/Remove Reviewed-By: Anna Henningsen Reviewed-By: Franziska Hinkelmann Reviewed-By: Tiancheng "Timothy" Gu Reviewed-By: James M Snell --- lib/internal/bootstrap/cache.js | 3 +- node.gyp | 3 +- src/env.cc | 9 +- src/env.h | 15 +- src/node.cc | 36 ++- src/node_code_cache.h | 19 -- src/node_code_cache_stub.cc | 22 +- src/node_internals.h | 5 + src/node_javascript.h | 41 --- src/node_native_module.cc | 324 +++++++++---------- src/node_native_module.h | 64 +++- src/node_union_bytes.h | 93 ++++++ test/code-cache/test-code-cache-generator.js | 10 +- test/code-cache/test-code-cache.js | 15 +- tools/generate_code_cache.js | 82 ++--- tools/js2c.py | 135 +++----- 16 files changed, 457 insertions(+), 419 deletions(-) delete mode 100644 src/node_code_cache.h delete mode 100644 src/node_javascript.h create mode 100644 src/node_union_bytes.h diff --git a/lib/internal/bootstrap/cache.js b/lib/internal/bootstrap/cache.js index ff6c4422c1148c..eb68b92ed866a6 100644 --- a/lib/internal/bootstrap/cache.js +++ b/lib/internal/bootstrap/cache.js @@ -9,11 +9,12 @@ const { NativeModule } = require('internal/bootstrap/loaders'); const { - source, + getSource, compileCodeCache } = internalBinding('native_module'); const { hasTracing } = process.binding('config'); +const source = getSource(); const depsModule = Object.keys(source).filter( (key) => NativeModule.isDepsModule(key) || key.startsWith('internal/deps') ); diff --git a/node.gyp b/node.gyp index b40933a0ebc0c4..d900ee63ed47f0 100644 --- a/node.gyp +++ b/node.gyp @@ -410,7 +410,6 @@ 'src/node_api.h', 'src/node_api_types.h', 'src/node_buffer.h', - 'src/node_code_cache.h', 'src/node_constants.h', 'src/node_contextify.h', 'src/node_errors.h', @@ -418,7 +417,6 @@ 'src/node_http2.h', 'src/node_http2_state.h', 'src/node_internals.h', - 'src/node_javascript.h', 'src/node_messaging.h', 'src/node_mutex.h', 'src/node_native_module.h', @@ -430,6 +428,7 @@ 'src/node_persistent.h', 'src/node_platform.h', 'src/node_root_certs.h', + 'src/node_union_bytes.h', 'src/node_version.h', 'src/node_watchdog.h', 'src/node_revert.h', diff --git a/src/env.cc b/src/env.cc index a39c51c26d17d7..5731bad9932603 100644 --- a/src/env.cc +++ b/src/env.cc @@ -1,13 +1,14 @@ -#include "node_internals.h" #include "async_wrap.h" -#include "v8-profiler.h" #include "node_buffer.h" -#include "node_platform.h" -#include "node_file.h" #include "node_context_data.h" +#include "node_file.h" +#include "node_internals.h" +#include "node_native_module.h" +#include "node_platform.h" #include "node_worker.h" #include "tracing/agent.h" #include "tracing/traced_value.h" +#include "v8-profiler.h" #include #include diff --git a/src/env.h b/src/env.h index 7f35f604946d68..6bed104dbb4f31 100644 --- a/src/env.h +++ b/src/env.h @@ -29,13 +29,13 @@ #include "inspector_agent.h" #endif #include "handle_wrap.h" +#include "node.h" +#include "node_http2_state.h" +#include "node_options.h" #include "req_wrap.h" #include "util.h" #include "uv.h" #include "v8.h" -#include "node.h" -#include "node_options.h" -#include "node_http2_state.h" #include #include @@ -347,12 +347,6 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2; V(libuv_stream_wrap_ctor_template, v8::FunctionTemplate) \ V(message_port, v8::Object) \ V(message_port_constructor_template, v8::FunctionTemplate) \ - V(native_modules_code_cache, v8::Object) \ - V(native_modules_code_cache_hash, v8::Object) \ - V(native_modules_source, v8::Object) \ - V(native_modules_source_hash, v8::Object) \ - V(native_modules_with_cache, v8::Set) \ - V(native_modules_without_cache, v8::Set) \ V(performance_entry_callback, v8::Function) \ V(performance_entry_template, v8::Function) \ V(pipe_constructor_template, v8::FunctionTemplate) \ @@ -684,6 +678,9 @@ class Environment { // List of id's that have been destroyed and need the destroy() cb called. inline std::vector* destroy_async_id_list(); + std::set native_modules_with_cache; + std::set native_modules_without_cache; + std::unordered_multimap hash_to_module_map; std::unordered_map id_to_module_map; std::unordered_map diff --git a/src/node.cc b/src/node.cc index faef976e974b4c..20aafe6a050e20 100644 --- a/src/node.cc +++ b/src/node.cc @@ -24,7 +24,6 @@ #include "node_context_data.h" #include "node_errors.h" #include "node_internals.h" -#include "node_javascript.h" #include "node_native_module.h" #include "node_perf.h" #include "node_platform.h" @@ -130,7 +129,7 @@ typedef int mode_t; namespace node { -using native_module::NativeModule; +using native_module::NativeModuleLoader; using options_parser::kAllowedInEnvironment; using options_parser::kDisallowedInEnvironment; using v8::Array; @@ -212,7 +211,7 @@ double prog_start_time; Mutex per_process_opts_mutex; std::shared_ptr per_process_opts { new PerProcessOptions() }; - +NativeModuleLoader per_process_loader; static Mutex node_isolate_mutex; static Isolate* node_isolate; @@ -1243,8 +1242,7 @@ static void GetInternalBinding(const FunctionCallbackInfo& args) { Null(env->isolate())).FromJust()); DefineConstants(env->isolate(), exports); } else if (!strcmp(*module_v, "natives")) { - exports = Object::New(env->isolate()); - NativeModule::GetNatives(env, exports); + exports = per_process_loader.GetSourceObject(env->context()); } else { return ThrowIfNoSuchModule(env, *module_v); } @@ -1780,18 +1778,24 @@ void LoadEnvironment(Environment* env) { // The bootstrapper scripts are lib/internal/bootstrap/loaders.js and // lib/internal/bootstrap/node.js, each included as a static C string - // defined in node_javascript.h, generated in node_javascript.cc by - // node_js2c. + // generated in node_javascript.cc by node_js2c. - // TODO(joyeecheung): use NativeModule::Compile + // TODO(joyeecheung): use NativeModuleLoader::Compile + // We duplicate the string literals here since once we refactor the bootstrap + // compilation out to NativeModuleLoader none of this is going to matter + Isolate* isolate = env->isolate(); Local loaders_name = - FIXED_ONE_BYTE_STRING(env->isolate(), "internal/bootstrap/loaders.js"); + FIXED_ONE_BYTE_STRING(isolate, "internal/bootstrap/loaders.js"); + Local loaders_source = + per_process_loader.GetSource(isolate, "internal/bootstrap/loaders"); MaybeLocal loaders_bootstrapper = - GetBootstrapper(env, LoadersBootstrapperSource(env), loaders_name); + GetBootstrapper(env, loaders_source, loaders_name); Local node_name = - FIXED_ONE_BYTE_STRING(env->isolate(), "internal/bootstrap/node.js"); + FIXED_ONE_BYTE_STRING(isolate, "internal/bootstrap/node.js"); + Local node_source = + per_process_loader.GetSource(isolate, "internal/bootstrap/node"); MaybeLocal node_bootstrapper = - GetBootstrapper(env, NodeBootstrapperSource(env), node_name); + GetBootstrapper(env, node_source, node_name); if (loaders_bootstrapper.IsEmpty() || node_bootstrapper.IsEmpty()) { // Execution was interrupted. @@ -1843,8 +1847,6 @@ void LoadEnvironment(Environment* env) { env->options()->debug_options->break_node_first_line) }; - NativeModule::LoadBindings(env); - // Bootstrap internal loaders Local bootstrapped_loaders; if (!ExecuteBootstrapper(env, loaders_bootstrapper.ToLocalChecked(), @@ -2485,7 +2487,6 @@ void FreePlatform(MultiIsolatePlatform* platform) { delete platform; } - Local NewContext(Isolate* isolate, Local object_template) { auto context = Context::New(isolate, nullptr, object_template); @@ -2499,8 +2500,9 @@ Local NewContext(Isolate* isolate, // Run lib/internal/per_context.js Context::Scope context_scope(context); - // TODO(joyeecheung): use NativeModule::Compile - Local per_context = NodePerContextSource(isolate); + // TODO(joyeecheung): use NativeModuleLoader::Compile + Local per_context = + per_process_loader.GetSource(isolate, "internal/per_context"); ScriptCompiler::Source per_context_src(per_context, nullptr); Local