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

build: speed up startup with V8 code cache #21405

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 16 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,22 @@ $(NODE_G_EXE): config.gypi out/Makefile
$(MAKE) -C out BUILDTYPE=Debug V=$(V)
if [ ! -r $@ -o ! -L $@ ]; then ln -fs out/Debug/$(NODE_EXE) $@; fi

CODE_CACHE_DIR ?= out/$(BUILDTYPE)/obj.target
CODE_CACHE_FILE ?= $(CODE_CACHE_DIR)/node_code_cache.cc
Copy link
Member

Choose a reason for hiding this comment

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

I think most all generated code is in out/$(BUILDTYPE)/obj/gen?

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax Yes, not sure why I picked obj.target...thanks for pointing that out, I'll fix it


.PHONY: with-code-cache
with-code-cache:
Copy link
Member Author

@joyeecheung joyeecheung Jun 19, 2018

Choose a reason for hiding this comment

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

It may be possible to put this into the gyp file, but given that gyp has not been supported by V8 and we are trying to migrate into a new build system, it's easier to put the two-pass build step into Makefile (but to support that on Windows we would need to port to vcbuild.bat)

It may also be possible to generate the file using an executable compiled from C++ that includes node_javascript.cc for the sources and calls V8 APIs to generate the cache instead of using a JS script, like the mk* targets in V8, but to do that we would still need to refactor the gyp file as well so I picked the easier route.

$(PYTHON) ./configure
$(MAKE)
mkdir -p $(CODE_CACHE_DIR)
out/$(BUILDTYPE)/$(NODE_EXE) --expose-internals tools/generate_code_cache.js $(CODE_CACHE_FILE)
$(PYTHON) ./configure --code-cache-path $(CODE_CACHE_FILE)
$(MAKE)

.PHONY: test-code-cache
test-code-cache: with-code-cache
$(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) code-cache

out/Makefile: common.gypi deps/uv/uv.gyp deps/http_parser/http_parser.gyp \
deps/zlib/zlib.gyp deps/v8/gypfiles/toolchain.gypi \
deps/v8/gypfiles/features.gypi deps/v8/gypfiles/v8.gyp node.gyp \
Expand Down
8 changes: 8 additions & 0 deletions configure
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,12 @@ parser.add_option('--without-snapshot',
dest='without_snapshot',
help=optparse.SUPPRESS_HELP)

parser.add_option('--code-cache-path',
action='store',
dest='code_cache_path',
help='Use a file generated by tools/generate_code_cache.js to compile the'
' code cache for builtin modules into the binary')

parser.add_option('--without-ssl',
action='store_true',
dest='without_ssl',
Expand Down Expand Up @@ -974,6 +980,8 @@ def configure_node(o):
o['variables']['debug_nghttp2'] = 'false'

o['variables']['node_no_browser_globals'] = b(options.no_browser_globals)
if options.code_cache_path:
o['variables']['node_code_cache_path'] = options.code_cache_path
o['variables']['node_shared'] = b(options.shared)
node_module_version = getmoduleversion.get_version()

Expand Down
60 changes: 58 additions & 2 deletions lib/internal/bootstrap/loaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,54 @@

const config = getBinding('config');

const codeCache = getInternalBinding('code_cache');
const compiledWithoutCache = [];
const compiledWithCache = [];

// Think of this as module.exports in this file even though it is not
// written in CommonJS style.
const loaderExports = { internalBinding, NativeModule };
const loaderId = 'internal/bootstrap/loaders';

// This is only exposed for internal build steps and testing purposes.
// We create new copies of the source and the code cache
// so the resources eventually used to compile builtin modules
// cannot be tampered with even with --expose-internals
const cacheExports = {
get builtinSource() {
return getBinding('natives');
},
get codeCache() {
return getInternalBinding('code_cache');
},
compiledWithoutCache,
compiledWithCache,
nativeModuleWrap(script) {
return NativeModule.wrap(script);
},
// Modules with source code compiled in js2c that
// cannot be compiled with the code cache
cannotUseCache: [
'config',
'internal/bootstrap/cache',
// TODO(joyeecheung): update the C++ side so that
// the code cache is also used when compiling these
// two files.
'internal/bootstrap/loaders',
'internal/bootstrap/node'
]
};
const cacheId = 'internal/bootstrap/cache';

NativeModule.require = function(id) {
if (id === loaderId) {
return loaderExports;
}

if (id === cacheId) {
return cacheExports;
}

const cached = NativeModule.getCached(id);
if (cached && (cached.loaded || cached.loading)) {
return cached.exports;
Expand Down Expand Up @@ -184,7 +223,7 @@
if (id === loaderId) {
return false;
}
return NativeModule.exists(id);
return id === cacheId || NativeModule.exists(id);
Copy link
Member

@jdalton jdalton Jun 25, 2018

Choose a reason for hiding this comment

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

Can it be handled as any other internal (in its own file)? That way it avoids conditional checks NativeModule.require and NativeModule.nonInternalExists.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jdalton Good idea, I'll do that

};

NativeModule.isInternal = function(id) {
Expand Down Expand Up @@ -229,7 +268,24 @@
this.loading = true;

try {
const script = new ContextifyScript(source, this.filename);
// (code, filename, lineOffset, columnOffset
// cachedData, produceCachedData, parsingContext)
const script = new ContextifyScript(
source, this.filename, 0, 0,
codeCache[this.id], false, undefined
);

// One of these conditions may be false when any of the inputs
// of the `node_js2c` target in node.gyp is modified.
// TODO(joyeecheung): figure out how to resolve the dependency
// issue. When the code cache is introduced we are at a point where
// refactoring node.gyp may not be worth the effort.
if (!codeCache[this.id] || script.cachedDataRejected) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should happen. If it does, something bad must have happened. Probably best to crash here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hashseed I assume you are talking about the second condition? The first happens when the cache is not built into the binary. Throwing on the second one (if (codeCache[this.id] && script.cachedDataRejected)) sounds reasonable to me.

compiledWithoutCache.push(this.id);
} else {
compiledWithCache.push(this.id);
}

// Arguments: timeout, displayErrors, breakOnSigint
const fn = script.runInThisContext(-1, true, false);
const requireFn = this.id.startsWith('internal/deps/') ?
Expand Down
7 changes: 7 additions & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'node_use_etw%': 'false',
'node_use_perfctr%': 'false',
'node_no_browser_globals%': 'false',
'node_code_cache_path%': '',
'node_use_v8_platform%': 'true',
'node_use_bundled_v8%': 'true',
'node_shared%': 'false',
Expand Down Expand Up @@ -396,6 +397,7 @@
'src/module_wrap.h',
'src/node.h',
'src/node_buffer.h',
'src/node_code_cache.h',
'src/node_constants.h',
'src/node_contextify.h',
'src/node_debug_options.h',
Expand Down Expand Up @@ -459,6 +461,11 @@
'NODE_OPENSSL_SYSTEM_CERT_PATH="<(openssl_system_ca_path)"',
],
'conditions': [
[ 'node_code_cache_path!=""', {
'sources': [ '<(node_code_cache_path)' ]
}, {
'sources': [ 'src/node_code_cache_stub.cc' ]
}],
[ 'node_shared=="true" and node_module_version!="" and OS!="win"', {
'product_extension': '<(shlib_suffix)',
'xcode_settings': {
Expand Down
13 changes: 11 additions & 2 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "node_buffer.h"
#include "node_constants.h"
#include "node_javascript.h"
#include "node_code_cache.h"
#include "node_platform.h"
#include "node_version.h"
#include "node_internals.h"
Expand Down Expand Up @@ -1595,10 +1596,18 @@ static void GetInternalBinding(const FunctionCallbackInfo<Value>& args) {

Local<String> module = args[0].As<String>();
node::Utf8Value module_v(env->isolate(), module);
Local<Object> exports;

node_module* mod = get_internal_module(*module_v);
if (mod == nullptr) return ThrowIfNoSuchModule(env, *module_v);
Local<Object> exports = InitModule(env, mod, module);
if (mod != nullptr) {
exports = InitModule(env, mod, module);
} else if (!strcmp(*module_v, "code_cache")) {
// internalBinding('code_cache')
exports = Object::New(env->isolate());
DefineCodeCache(env, exports);
} else {
return ThrowIfNoSuchModule(env, *module_v);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a functional difference here?

Copy link
Member Author

@joyeecheung joyeecheung Jun 19, 2018

Choose a reason for hiding this comment

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

@TimothyGu I was adding the code_cache binding to internalBindnig but later moved that to process.binding for debugging purposes. This is the legacy of that cut-and-paste. I think we should move it back in the final incarnation of this PR..


args.GetReturnValue().Set(exports);
}
Expand Down
16 changes: 16 additions & 0 deletions src/node_code_cache.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
#ifndef SRC_NODE_CODE_CACHE_H_
#define SRC_NODE_CODE_CACHE_H_

#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#include "node_internals.h"

namespace node {

void DefineCodeCache(Environment* env, v8::Local<v8::Object> target);

} // namespace node

#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

#endif // SRC_NODE_CODE_CACHE_H_
14 changes: 14 additions & 0 deletions src/node_code_cache_stub.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

#include "node_code_cache.h"

// This is supposed to be generated by tools/generate_code_cache.js
// The stub here is used when configure is run without `--code-cache-path`

namespace node {
void DefineCodeCache(Environment* env, v8::Local<v8::Object> target) {
// When we do not produce code cache for builtin modules,
// `internalBinding('code_cache')` returns an empty object
// (here as `target`) so this is a noop.
}

} // namespace node
21 changes: 21 additions & 0 deletions test/code-cache/code-cache.status
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
prefix v8-updates
Copy link
Member

Choose a reason for hiding this comment

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

prefix code-cache? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

oops, copy paste error :P


# To mark a test as flaky, list the test name in the appropriate section
# below, without ".js", followed by ": PASS,FLAKY". Example:
# sample-test : PASS,FLAKY

[true] # This section applies to all platforms

[$system==win32]

[$system==linux]

[$system==macos]

[$arch==arm || $arch==arm64]

[$system==solaris] # Also applies to SmartOS

[$system==freebsd]

[$system==aix]
42 changes: 42 additions & 0 deletions test/code-cache/test-code-cache.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
'use strict';

// Flags: --expose-internals
// This test verifies that the binary is compiled with code cache and the
// cache is used when built in modules are compiled.

require('../common');
const assert = require('assert');
const {
types: {
isUint8Array
}
} = require('util');
const {
builtinSource,
codeCache,
cannotUseCache,
compiledWithCache,
compiledWithoutCache
} = require('internal/bootstrap/cache');

assert.strictEqual(
typeof process.config.variables.node_code_cache_path,
'string'
);

assert.deepStrictEqual(compiledWithoutCache, []);

const loadedModules = process.moduleLoadList
.filter((m) => m.startsWith('NativeModule'))
.map((m) => m.replace('NativeModule ', ''));

for (const key of loadedModules) {
assert(compiledWithCache.includes(key),
`"${key}" should've been compiled with code cache`);
}

for (const key of Object.keys(builtinSource)) {
if (cannotUseCache.includes(key)) continue;
assert(isUint8Array(codeCache[key]) && codeCache[key].length > 0,
`Code cache for "${key}" should've been generated`);
}
6 changes: 6 additions & 0 deletions test/code-cache/testcfg.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import sys, os
sys.path.append(os.path.join(os.path.dirname(__file__), '..'))
import testpy

def GetConfiguration(context, root):
return testpy.ParallelTestConfiguration(context, root, 'code-cache')
Loading