src: compile native modules and their code cache in C++ #24221

joyeecheung · 2018-11-07T10:57:09Z

This PR contains two patch. The first one, a benchmark patch, is only here so that we can run the benchmark CI with more options. I have opened another PR for that patch: #24220

The second patch moves the native module compilation to C++.
Local benchmark results:

                                                                                   confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***      4.35 %       ±1.62% ±2.16% ±2.82%
 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***      3.80 %       ±1.20% ±1.60% ±2.09%
 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***      6.77 %       ±2.24% ±2.98% ±3.89%
 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                      **      3.85 %       ±2.23% ±2.96% ±3.86%

src: compile native modules and their code cache in C++

This patch refactors out a part of NativeModule.prototype.compile
(in JS land) into a C++ NativeModule class, this enables a
couple of possibilities:

By moving the code to the C++ land, we have more opportunity
to specialize the compilation process of the native modules
(e.g. compilation options, code cache) that is orthogonal to
how user land modules are compiled
We can reuse the code to compile bootstrappers and context
fixers and enable them to be compiled with the code cache later,
since they are not loaded by NativeModule in the JS land their
caching must be done in C++.
Since there is no need to pass the static data to JS for
compilation anymore, this enables us to use
(std::map<std::string, const char*>) in the generated
node_code_cache.cc and node_javascript.cc later, and scope
every actual access to the source of native modules to a
std::map lookup instead of a lookup on a v8::Object in
dictionary mode.

This patch also refactor the code cache generator and tests
a bit and trace the withCodeCache and withoutCodeCache
in a Set instead of an Array, and makes sure that all the cachable
builtins are tested.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2018-11-07T10:57:10Z

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1526/pipeline

joyeecheung · 2018-11-07T10:59:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18401/
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/260/

This supersedes #23837 cc @ryzokuken

cc @addaleax @jasnell

joyeecheung · 2018-11-07T11:00:58Z

Also cc @bcoe to confirm

This potentially also enables us to dispatch Profiler.startPreciseCoverage prior to running the bootstrappers.

joyeecheung · 2018-11-07T15:47:49Z

Last one had infra issues, resumed build: https://ci.nodejs.org/job/node-test-pull-request/18403/

joyeecheung · 2018-11-07T17:06:19Z

Testing CI is green. Not sure why but there is a even bigger improvement on Linux when running workers with the extra requires. Benchmark CI results.

Benchmark results

                                                                                   confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***      4.21 %       ±1.46% ±1.95% ±2.55%
 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***      2.13 %       ±1.11% ±1.47% ±1.92%
 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***     11.31 %       ±1.33% ±1.77% ±2.32%
 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***      3.14 %       ±0.85% ±1.13% ±1.47%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 4 comparisons, you can thus
expect the following amount of false-positive results:
  0.20 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.04 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)
Notifying upstream projects of job completion
Finished: SUCCESS

Significant impact

                                                                                   confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***      4.21 %       ±1.46% ±1.95% ±2.55%
 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***      2.13 %       ±1.11% ±1.47% ±1.92%
 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***     11.31 %       ±1.33% ±1.77% ±2.32%
 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***      3.14 %       ±0.85% ±1.13% ±1.47%

bcoe · 2018-11-07T18:00:44Z

@joyeecheung #23941 got coverage being enabled early enough that missing coverage for internal modules is no longer an issue 👍

joyeecheung · 2018-11-08T08:30:46Z

@bcoe Thanks, judging from https://coverage.nodejs.org/coverage-7cf56797dddc7a00/root/internal/bootstrap/loaders.js.html looks like it already works with the current approach, so I'll remove that part as a goal.

This patch refactors out a part of NativeModule.prototype.compile (in JS land) into a C++ NativeModule class, this enables a couple of possibilities: 1. By moving the code to the C++ land, we have more opportunity to specialize the compilation process of the native modules (e.g. compilation options, code cache) that is orthogonal to how user land modules are compiled 2. We can reuse the code to compile bootstrappers and context fixers and enable them to be compiled with the code cache later, since they are not loaded by NativeModule in the JS land their caching must be done in C++. 3. Since there is no need to pass the static data to JS for compilation anymore, this enables us to use (std::map<std::string, const char*>) in the generated node_code_cache.cc and node_javascript.cc later, and scope every actual access to the source of native modules to a std::map lookup instead of a lookup on a v8::Object in dictionary mode. This patch also refactor the code cache generator and tests a bit and trace the `withCodeCache` and `withoutCodeCache` in a Set instead of an Array, and makes sure that all the cachable builtins are tested.

joyeecheung · 2018-11-13T20:03:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18586/

joyeecheung · 2018-11-13T20:04:51Z

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/273/

joyeecheung · 2018-11-13T21:13:08Z

Benchmark results from the CI

                                                                                   confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js mode='process' script='benchmark/fixtures/require-cachable' dur=1        ***      9.74 %       ±1.74% ±2.32% ±3.02%
 misc/startup.js mode='process' script='test/fixtures/semicolon' dur=1                    ***      2.25 %       ±1.28% ±1.71% ±2.23%
 misc/startup.js mode='worker' script='benchmark/fixtures/require-cachable' dur=1         ***     11.85 %       ±1.38% ±1.84% ±2.41%
 misc/startup.js mode='worker' script='test/fixtures/semicolon' dur=1                     ***      2.98 %       ±1.18% ±1.57% ±2.05%

@nodejs/process Can I get some review please?

TimothyGu · 2018-11-13T21:42:57Z

src/env.h

+  V(native_module_code_cache_hash, v8::Object)                                 \
+  V(native_module_column_offset, v8::Integer)                                  \
+  V(native_module_line_offset, v8::Integer)                                    \
+  V(native_module_parameters, v8::Array)                                       \


This doesn't seem to be used anywhere.

TimothyGu · 2018-11-13T21:44:04Z

src/env.h

+  V(native_module_parameters, v8::Array)                                       \
+  V(native_module_source, v8::Object)                                          \
+  V(native_module_source_hash, v8::Object)                                     \
+  V(native_module_with_cache, v8::Set)                                         \


Nit:

Suggested change

V(native_module_with_cache, v8::Set) \

V(native_modules_with_cache, v8::Set) \

src/node_native_module.cc

TimothyGu · 2018-11-13T21:53:35Z

src/node_native_module.cc

+  env->set_native_module_code_cache_hash(native_module_code_cache_hash);
+
+  env->set_native_module_column_offset(Integer::New(isolate, 0));
+  env->set_native_module_line_offset(Integer::New(isolate, 0));


Any reason why we have these two variables rather than just hardcoding 0? Integer::New takes next to no time anyway.

I wasn't sure about the offset of bootstrappers - looks like they are also 0-offsetted, so hard coding should be fine.

TimothyGu · 2018-11-13T21:55:06Z

test/code-cache/test-code-cache.js

 } else {
-  // The binary is configured with code cache.
+  console.log('The binary is configured with code cache');


Suggested change

console.log('The binary is configured with code cache');

// The binary is configured with code cache.

😉

I prefer to see this when I run the tests by hand, just so that I don't miss anything because this test is very fail-safe. Do you have a particular reason why we can't console log here?

TimothyGu · 2018-11-13T21:58:30Z

src/node_native_module.cc

+  }
+
+  CHECK(result->IsUint8Array());
+  Local<Uint8Array> code_cache = result.As<Uint8Array>();


Replace this and the later ArrayBuffer::Contents unwrapping with SPREAD_BUFFER_ARG?

node/src/util.h

Lines 385 to 394 in 0603c0a

#define SPREAD_BUFFER_ARG(val, name) \

CHECK((val)->IsArrayBufferView()); \

v8::Local<v8::ArrayBufferView> name = (val).As<v8::ArrayBufferView>(); \

v8::ArrayBuffer::Contents name##_c = name->Buffer()->GetContents(); \

const size_t name##_offset = name->ByteOffset(); \

const size_t name##_length = name->ByteLength(); \

char* const name##_data = \

static_cast<char*>(name##_c.Data()) + name##_offset; \

if (name##_length > 0) \

CHECK_NE(name##_data, nullptr);

The API takes uint8_t*, there is no stopping you doing a reinterpret_cast I guess, but is there any real benefit in using a macro to eliminate two lines and then reinterpret_cast?

Personally I prefer two lines without a reinterpret_cast (definatly over a macro).

TimothyGu · 2018-11-13T21:59:03Z

src/node_native_module.cc

+  Isolate* isolate = env->isolate();
+
+  Local<String> source;
+  Local<Uint8Array> code_cache;


Seems unused.

TimothyGu · 2018-11-13T21:59:22Z

src/node_native_module.cc

+  Local<Value> result;
+  result = env->native_module_source()->Get(context, id).ToLocalChecked();
+  CHECK(result->IsString());
+  source = result.As<String>();


Can we just declare Local<String> source here?

TimothyGu · 2018-11-13T21:59:30Z

src/node_native_module.cc

+  Local<Uint8Array> code_cache;
+
+  Local<Value> result;
+  result = env->native_module_source()->Get(context, id).ToLocalChecked();


Coalesce these two lines?

TimothyGu · 2018-11-13T22:00:18Z

src/node_native_module.cc

+
+  env->SetMethod(target, "compileFunction", NativeModule::CompileFunction);
+  env->SetMethod(target, "compileCodeCache", NativeModule::CompileCodeCache);
+  // internalBinding('native_module') should be forzen


nit:

Suggested change

// internalBinding('native_module') should be forzen

// internalBinding('native_module') should be frozen

src/node_native_module.cc

refack

🎉 🎉 🎉

joyeecheung · 2018-11-14T01:21:44Z

Fixed some of the nits: https://ci.nodejs.org/job/node-test-pull-request/18596/

joyeecheung · 2018-11-14T11:09:03Z

I plan to land this later today, in order to pursue 2 and 3 listed in the OP.
On a side note, in my prototype std::map<std::string, const char*> is not quite enough to store all the information for the native modules, we need to make a special struct that has a union of const uint8_t* and const uint16_t* for two types of strings, but it seems fairly doable and makes it possible to encapsulate more code into non-generated C++ files - the generated files essentially only declares a bunch of static const uintx_t[] and does map.insert(), and it's not too hard to read them if you just tail the initializers. (if we really really want to optimize the look up we can even implement a trie, though I am not sure if that gives enough benefit than std::map considering there are only < 200 entries)
This also makes the native module map fairly air-tight, since there is no way to access that std::map from JS except to retrieve constant strings via process.binding('natives').

addaleax · 2018-11-14T12:03:50Z

src/env.h

+  V(trace_category_state_function, v8::Function)                               \
+  V(tty_constructor_template, v8::FunctionTemplate)                            \
+  V(udp_constructor_function, v8::Function)                                    \
+  V(url_constructor_function, v8::Function)                                    \


Can we backport this part immediately after the PR lands, or split it into its own PR?

I can try clang-format this in 10.x, not sure if it's necessary for 8.x

addaleax · 2018-11-14T12:04:56Z

src/node_code_cache.h

@@ -7,6 +7,8 @@

 namespace node {

+extern bool native_module_has_code_cache;


Can this be const, to clarify that it’s not global mutable state?

Even constexpr.
Actually it probably should move to .gyp... I need to keep this in mind

@refack This is the one bit that we can generate without a chicken-and-egg problem...what what good does it do if we can't generate the actual static data along with it?

Although, come to think of it, now that we refactored the native module compilation/code cache generation in C++, we may be able to solve that chicken-and-egg problem more easily...when I manage to migrate the map to a std::map, I believe we only have a dependency of a working v8 isolate to convert source code into code cache, that's easier to plug in than node..

@refack This is the one bit that we can generate without a chicken-and-egg problem...what what good does it do if we can't generate the actual static data along with it?

I mean, GYP knows if the code cache is a stub or real (if node_code_cache_stub.cc is included or generated/node_code_cache.cc) - So it can inject #define HAS_REAL_CODE_CACHE 0 / 1 and we don't need this as code at all.

P.S. we need to push for GCC7 and get if constexpr that way we can write code that doesn't compile decided by const code values.

addaleax · 2018-11-14T12:06:24Z

src/node_native_module.cc

+        ScriptCompiler::CreateCodeCacheForFunction(fun));
+    CHECK_NE(cached_data, nullptr);
+    char* data =
+        reinterpret_cast<char*>(const_cast<uint8_t*>(cached_data->data));


I don’t think you need to cast the const away?

I do...otherwise the compiler barks at me..

Hm … my compiler (gcc 7.3.0) accepts this:

diff --git a/src/node_native_module.cc b/src/node_native_module.cc index 50f9e3dc3a53..d6b938309d5c 100644 --- a/src/node_native_module.cc +++ b/src/node_native_module.cc @@ -262,8 +262,7 @@ Local<Value> NativeModule::Compile(Environment* env, std::unique_ptr<ScriptCompiler::CachedData> cached_data( ScriptCompiler::CreateCodeCacheForFunction(fun)); CHECK_NE(cached_data, nullptr); - char* data = - reinterpret_cast<char*>(const_cast<uint8_t*>(cached_data->data)); + const char* data = reinterpret_cast<const char*>(cached_data->data); // Since we have no API to create a buffer from a new'ed pointer, // we will need to copy it - but this code path is only run by the // tooling that generates the code cache to be bundled in the binary

So it sounds like a compiler bug? Maybe we could leave a comment so that people aren’t tempted to refactor the const_cast away?

Just FTR, mutating the value of cast-away-const is undefined behaviour, and in recent MSVS version, it will probably crash (definatly crash in a Debug build).
So we might want to consider memcpy this, or CL ustream to mark it mutable

There is no mutation of the underlying data – the const_cast is valid, if it is necessary.

If const_cast actually causes crashes when its valid, this is probably one of the last place we need to worry about :) (it's only invoked by code cache generator, which hopefully no-one but us uses)

joyeecheung · 2018-11-14T16:14:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/18611/

refack · 2018-11-14T16:59:25Z

we need to make a special struct that has a union of const uint8_t* and const uint16_t* for two types of strings,

Will it be simpler to upfront encode all strings as uint16_t* (or char16_t*)?

joyeecheung · 2018-11-14T17:07:00Z

Will it be simpler to upfront encode all strings as uint16_t* (or char16_t*)?

We still need to teach V8 whether its one byte or two byte when we need to convert them to external strings

joyeecheung · 2018-11-14T18:12:21Z

CI is green: somehow https://ci.nodejs.org/job/node-test-binary-windows/21540/ is having trouble notifying upstream..

joyeecheung · 2018-11-14T18:31:40Z

Landed in bd765d6

This patch refactors out a part of NativeModule.prototype.compile (in JS land) into a C++ NativeModule class, this enables a couple of possibilities: 1. By moving the code to the C++ land, we have more opportunity to specialize the compilation process of the native modules (e.g. compilation options, code cache) that is orthogonal to how user land modules are compiled 2. We can reuse the code to compile bootstrappers and context fixers and enable them to be compiled with the code cache later, since they are not loaded by NativeModule in the JS land their caching must be done in C++. 3. Since there is no need to pass the static data to JS for compilation anymore, this enables us to use (std::map<std::string, const char*>) in the generated node_code_cache.cc and node_javascript.cc later, and scope every actual access to the source of native modules to a std::map lookup instead of a lookup on a v8::Object in dictionary mode. This patch also refactor the code cache generator and tests a bit and trace the `withCodeCache` and `withoutCodeCache` in a Set instead of an Array, and makes sure that all the cachable builtins are tested. PR-URL: #24221 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

joyeecheung · 2018-11-24T01:03:55Z

Notes: to backport this change, these changes need to be backported first

And there are several changes not started by me but should to be backported if we want to do this properly without causing another bunch of conflicts

ryzokuken · 2018-11-25T08:45:14Z

We still need to teach V8 whether its one byte or two byte when we need to convert them to external strings

And we cannot use v8::String?

joyeecheung · 2018-11-25T10:15:05Z

@ryzokuken We can, but the underlying data is independent of any V8 isolate in nature, UnionBytes::ToStringChecked just materializes that external data with a V8 isolate, you still have to store the actual data somewhere. Also storing them all as uint16_t is a bit of a waste of memory as the majority of the source code under lib are all-ASCII...I guess you can pack two uint8_t into one uint16_t and when materializing them just reinterpret_cast, but simply using a union is much safer and more readable.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 7, 2018

joyeecheung force-pushed the native-module branch from 2d84ae0 to 56fb2ab Compare November 13, 2018 20:01

TimothyGu reviewed Nov 13, 2018

View reviewed changes

refack reviewed Nov 14, 2018

View reviewed changes

src/node_native_module.cc Show resolved Hide resolved

refack approved these changes Nov 14, 2018

View reviewed changes

fixup! src: compile native modules and their code cache in C++

7f8e120

addaleax approved these changes Nov 14, 2018

View reviewed changes

fixup! fixup! src: compile native modules and their code cache in C++

c7b7706

joyeecheung closed this Nov 14, 2018

joyeecheung added backport-requested-v10.x process Issues and PRs related to the process subsystem. labels Nov 14, 2018

BridgeAR mentioned this pull request Nov 15, 2018

Release proposal: v11.2 #24350

Merged

	V(native_module_with_cache, v8::Set) \
	V(native_modules_with_cache, v8::Set) \

	console.log('The binary is configured with code cache');
	// The binary is configured with code cache.

	#define SPREAD_BUFFER_ARG(val, name) \
	CHECK((val)->IsArrayBufferView()); \
	v8::Local<v8::ArrayBufferView> name = (val).As<v8::ArrayBufferView>(); \
	v8::ArrayBuffer::Contents name##_c = name->Buffer()->GetContents(); \
	const size_t name##_offset = name->ByteOffset(); \
	const size_t name##_length = name->ByteLength(); \
	char* const name##_data = \
	static_cast<char*>(name##_c.Data()) + name##_offset; \
	if (name##_length > 0) \
	CHECK_NE(name##_data, nullptr);

	// internalBinding('native_module') should be forzen
	// internalBinding('native_module') should be frozen

		@@ -7,6 +7,8 @@

		namespace node {

		extern bool native_module_has_code_cache;

src: compile native modules and their code cache in C++ #24221

src: compile native modules and their code cache in C++ #24221

Conversation

joyeecheung commented Nov 7, 2018 • edited Loading

src: compile native modules and their code cache in C++

Checklist

nodejs-github-bot commented Nov 7, 2018

joyeecheung commented Nov 7, 2018

joyeecheung commented Nov 7, 2018

joyeecheung commented Nov 7, 2018

joyeecheung commented Nov 7, 2018 • edited Loading

bcoe commented Nov 7, 2018

joyeecheung commented Nov 8, 2018

joyeecheung commented Nov 13, 2018

joyeecheung commented Nov 13, 2018

joyeecheung commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment

Choose a reason for hiding this comment

joyeecheung commented Nov 14, 2018

joyeecheung commented Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Nov 14, 2018

refack commented Nov 14, 2018

joyeecheung commented Nov 14, 2018

joyeecheung commented Nov 14, 2018

joyeecheung commented Nov 14, 2018

joyeecheung commented Nov 24, 2018 • edited Loading

ryzokuken commented Nov 25, 2018

joyeecheung commented Nov 25, 2018 • edited Loading

joyeecheung commented Nov 7, 2018 •

edited

Loading

joyeecheung commented Nov 7, 2018 •

edited

Loading

refack Nov 14, 2018 •

edited

Loading

joyeecheung commented Nov 14, 2018 •

edited

Loading

joyeecheung Nov 14, 2018 •

edited

Loading

refack Nov 14, 2018 •

edited

Loading

refack Nov 14, 2018 •

edited

Loading

joyeecheung commented Nov 24, 2018 •

edited

Loading

joyeecheung commented Nov 25, 2018 •

edited

Loading