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

lib: fix code cache generation, add more tests and enable them by default #23855

Closed
wants to merge 4 commits into from
Closed
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
12 changes: 11 additions & 1 deletion lib/internal/bootstrap/cache.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@
// cannot be tampered with even with --expose-internals

const {
NativeModule, internalBinding
NativeModule
} = require('internal/bootstrap/loaders');
const { hasTracing } = process.binding('config');

function getCodeCache(id) {
const cached = NativeModule.getCached(id);
Expand Down Expand Up @@ -42,6 +43,15 @@ const cannotUseCache = [
'internal/bootstrap/node'
].concat(depsModule);

if (process.config.variables.v8_enable_inspector !== 1) {
Copy link
Member

Choose a reason for hiding this comment

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

May be worthwhile including a quick comment with these that says why they cannot use the cache, just so it's clear to folks coming along later.

cannotUseCache.push('inspector');
cannotUseCache.push('internal/util/inspector');
}

if (!hasTracing) {
cannotUseCache.push('trace_events');
}

module.exports = {
cachableBuiltins: Object.keys(NativeModule._source).filter(
(key) => !cannotUseCache.includes(key)
Expand Down
58 changes: 58 additions & 0 deletions test/code-cache/test-code-cache-generator.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
'use strict';

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

const common = require('../common');

const tmpdir = require('../common/tmpdir');
const { spawnSync } = require('child_process');
const assert = require('assert');
const path = require('path');
const fs = require('fs');
const readline = require('readline');

const generator = path.join(
__dirname, '..', '..', 'tools', 'generate_code_cache.js'
);
tmpdir.refresh();
const dest = path.join(tmpdir.path, 'cache.cc');

// Run tools/generate_code_cache.js
const child = spawnSync(
process.execPath,
['--expose-internals', generator, dest]
);
assert.ifError(child.error);
if (child.status !== 0) {
console.log(child.stderr.toString());
assert.strictEqual(child.status, 0);
}

// Verifies that:
// - node::DefineCodeCache()
// - node::DefineCodeCacheHash()
// are defined in the generated code.
// See src/node_code_cache_stub.cc for explanations.

const rl = readline.createInterface({
input: fs.createReadStream(dest),
crlfDelay: Infinity
});

let hasCacheDef = false;
let hasHashDef = false;

rl.on('line', common.mustCallAtLeast((line) => {
if (line.includes('DefineCodeCache(')) {
hasCacheDef = true;
}
if (line.includes('DefineCodeCacheHash(')) {
hasHashDef = true;
}
}, 2));

rl.on('close', common.mustCall(() => {
assert.ok(hasCacheDef);
assert.ok(hasHashDef);
}));
46 changes: 30 additions & 16 deletions test/code-cache/test-code-cache.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
'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.
// This test verifies that if the binary is compiled with code cache,
// and the cache is used when built in modules are compiled.
// Otherwise, verifies that no cache is used when compiling builtins.

require('../common');
const assert = require('assert');
Expand All @@ -18,23 +19,36 @@ const {
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`);
}
// The binary is not configured with code cache, verifies that the builtins
// are all compiled without cache and we are doing the bookkeeping right.
if (process.config.variables.node_code_cache_path === undefined) {
assert.deepStrictEqual(compiledWithCache, []);
assert.notStrictEqual(compiledWithoutCache.length, 0);

for (const key of loadedModules) {
assert(compiledWithoutCache.includes(key),
`"${key}" should not have been compiled with code cache`);
}

for (const key of cachableBuiltins) {
assert(isUint8Array(codeCache[key]) && codeCache[key].length > 0,
`Code cache for "${key}" should've been generated`);
} else {
// The binary is configured with code cache.
assert.strictEqual(
typeof process.config.variables.node_code_cache_path,
'string'
);
assert.deepStrictEqual(compiledWithoutCache, []);

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

for (const key of cachableBuiltins) {
assert(isUint8Array(codeCache[key]) && codeCache[key].length > 0,
`Code cache for "${key}" should've been generated`);
}
}
1 change: 0 additions & 1 deletion tools/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -1498,7 +1498,6 @@ def PrintCrashed(code):
IGNORED_SUITES = [
'addons',
'addons-napi',
'code-cache',
'doctool',
'internet',
'pummel',
Expand Down