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
feat: various v8 snapshot improvements #24909
Changes from 44 commits
3f3e308
7d65a96
fcfa789
90c96c6
5493177
474b3a4
e6cde79
ac5ebdc
25b870c
968e726
51426dd
c071402
8ab76e6
b4e2261
df39189
239a953
b4f8119
b77cf7c
e2dd80f
dc0332a
6b5185c
5e6f47d
42f8935
0c2dfae
d48caa8
528d91b
9a4157e
35678ef
cb7e3b2
1a4c521
1c1ce89
76a18b6
1720cb8
3ea52dc
250e9f8
9ba3016
35c8b6f
401fbdc
d3c832a
eb0dcfe
f27b687
b199681
bd533d3
8d2598a
39d02d6
ce8c74a
fe8b6cd
bb1390e
aa6b359
4187ff6
ab63313
6751287
bebcd8a
30660b0
5c1f981
adadd79
5f3a735
12a5753
5442930
4e8d8e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,13 +3,8 @@ if (process.env.CYPRESS_INTERNAL_ENV === 'production') { | |
throw new Error(`${__filename} should only run outside of prod`) | ||
} | ||
|
||
if (require.name !== 'customRequire') { | ||
// Purposefully make this a dynamic require so that it doesn't have the potential to get picked up by snapshotting mechanism | ||
const hook = './hook' | ||
const { hookRequire } = require('@packages/server/hook-require') | ||
|
||
const { hookRequire } = require(`@packages/server/${hook}-require`) | ||
|
||
hookRequire(true) | ||
} | ||
hookRequire({ forceTypeScript: true }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how do we know to force typescript? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This file never runs in prod (see line 2). It only runs in development where we still need to force typescript compilation on the fly. |
||
|
||
require('../lib/threads/worker.ts') |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,19 @@ | ||
const { initializeStartTime } = require('./lib/util/performance_benchmark') | ||
|
||
const run = async () => { | ||
initializeStartTime() | ||
try { | ||
initializeStartTime() | ||
|
||
if (require.name !== 'customRequire') { | ||
// Purposefully make this a dynamic require so that it doesn't have the potential to get picked up by snapshotting mechanism | ||
const hook = './hook' | ||
const { hookRequire } = require('./hook-require') | ||
|
||
const { hookRequire } = require(`${hook}-require`) | ||
hookRequire({ forceTypeScript: false }) | ||
|
||
hookRequire(false) | ||
await require('./server-entry') | ||
} catch (error) { | ||
// eslint-disable-next-line no-console | ||
console.error(error) | ||
process.exit(1) | ||
} | ||
|
||
await require('./server-entry') | ||
} | ||
|
||
module.exports = run() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
require('./server-entry') |
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -7,6 +7,7 @@ const path = require('path') | |||||||||||
const { setupV8Snapshots } = require('@tooling/v8-snapshot') | ||||||||||||
const { flipFuses, FuseVersion, FuseV1Options } = require('@electron/fuses') | ||||||||||||
const { cleanup } = require('./binary/binary-cleanup') | ||||||||||||
const { getIntegrityCheckSource, getBinaryEntryPointSource } = require('./binary/binary-sources') | ||||||||||||
|
||||||||||||
module.exports = async function (params) { | ||||||||||||
console.log('****************************') | ||||||||||||
|
@@ -55,6 +56,8 @@ module.exports = async function (params) { | |||||||||||
} | ||||||||||||
|
||||||||||||
if (!['1', 'true'].includes(process.env.DISABLE_SNAPSHOT_REQUIRE)) { | ||||||||||||
await fs.writeFile(path.join(outputFolder, 'index.js'), getBinaryEntryPointSource()) | ||||||||||||
|
||||||||||||
await flipFuses( | ||||||||||||
exePathPerPlatform[os.platform()], | ||||||||||||
{ | ||||||||||||
|
@@ -63,7 +66,7 @@ module.exports = async function (params) { | |||||||||||
}, | ||||||||||||
) | ||||||||||||
|
||||||||||||
await setupV8Snapshots(params.appOutDir) | ||||||||||||
await cleanup(outputFolder) | ||||||||||||
await setupV8Snapshots({ cypressAppPath: params.appOutDir, integrityCheckSource: getIntegrityCheckSource(outputFolder) }) | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did this move after There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll comment in this file to clarify. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commented: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated: |
||||||||||||
} | ||||||||||||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,6 +6,7 @@ const esbuild = require('esbuild') | |||||
const snapshotMetadata = require('@tooling/v8-snapshot/cache/prod-darwin/snapshot-meta.cache.json') | ||||||
const tempDir = require('temp-dir') | ||||||
const workingDir = path.join(tempDir, 'binary-cleanup-workdir') | ||||||
const bytenode = require('bytenode') | ||||||
|
||||||
fs.ensureDirSync(workingDir) | ||||||
|
||||||
|
@@ -39,8 +40,6 @@ async function removeEmptyDirectories (directory) { | |||||
const getDependencyPathsToKeep = async (buildAppDir) => { | ||||||
const unixBuildAppDir = buildAppDir.split(path.sep).join(path.posix.sep) | ||||||
const startingEntryPoints = [ | ||||||
'packages/server/index.js', | ||||||
'packages/server/hook-require.js', | ||||||
'packages/server/lib/plugins/child/require_async_child.js', | ||||||
'packages/server/lib/plugins/child/register_ts_node.js', | ||||||
'packages/rewriter/lib/threads/worker.js', | ||||||
|
@@ -114,13 +113,53 @@ const getDependencyPathsToKeep = async (buildAppDir) => { | |||||
return [...Object.keys(esbuildResult.metafile.inputs), ...entryPoints] | ||||||
} | ||||||
|
||||||
const createServerEntryPointBundle = async (buildAppDir) => { | ||||||
const unixBuildAppDir = buildAppDir.split(path.sep).join(path.posix.sep) | ||||||
const entryPoints = [path.join(unixBuildAppDir, 'packages/server/index.js')] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am trying to limit node internals references as much as I possibly can. See: and |
||||||
// Build the binary entry point ignoring anything that happens in the server-entry since that will be in the v8 snapshot | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the naming a little bit. I agree it's confusing as is: |
||||||
const esbuildResult = await esbuild.build({ | ||||||
ryanthemanuel marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
entryPoints, | ||||||
bundle: true, | ||||||
outdir: workingDir, | ||||||
platform: 'node', | ||||||
metafile: true, | ||||||
absWorkingDir: unixBuildAppDir, | ||||||
external: [ | ||||||
'./transpile-ts', | ||||||
'./server-entry', | ||||||
], | ||||||
}) | ||||||
|
||||||
console.log(`copying server entry point bundle from ${path.join(workingDir, 'index.js')} to ${path.join(buildAppDir, 'packages', 'server', 'index.js')}`) | ||||||
|
||||||
await fs.copy(path.join(workingDir, 'index.js'), path.join(buildAppDir, 'packages', 'server', 'index.js')) | ||||||
|
||||||
console.log(`compiling server entry point bundle to ${path.join(buildAppDir, 'packages', 'server', 'index.jsc')}`) | ||||||
|
||||||
// Use bytenode to compile the entry point bundle. This will save time on the v8 compile step and ensure the integrity of the entry point | ||||||
await bytenode.compileFile({ | ||||||
ryanthemanuel marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
filename: path.join(buildAppDir, 'packages', 'server', 'index.js'), | ||||||
output: path.join(buildAppDir, 'packages', 'server', 'index.jsc'), | ||||||
electron: true, | ||||||
}) | ||||||
|
||||||
return [...Object.keys(esbuildResult.metafile.inputs)].map((input) => `./${input}`) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be a hard-coded separator? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the way these files are read in assumes "require" syntax not platform specific syntax. |
||||||
} | ||||||
|
||||||
const cleanup = async (buildAppDir) => { | ||||||
// 1. Retrieve all dependencies that still need to be kept in the binary. In theory, we could use the bundles generated here as single files within the binary, | ||||||
// but for now, we just track on the dependencies that get pulled in | ||||||
const keptDependencies = [...await getDependencyPathsToKeep(buildAppDir), 'package.json', 'packages/server/server-entry.js'] | ||||||
const keptDependencies = [...await getDependencyPathsToKeep(buildAppDir), 'package.json'] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use promise.all for getting getDependencyPathsToKeep & createServerEntryPointBundle? const [
keptDependencies,
serverPointEntries,
] = Promise.all(getDependencyPathsToKeep(buildAppDir), createServerEntryPointBundle(buildAppDir)] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this idea. Will work on it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated: |
||||||
|
||||||
// 2. Create the entry point bundle and then gather the dependencies that could potentially be removed from the binary due to being in the snapshot or in the entry point bundle | ||||||
const potentiallyRemovedDependencies = [ | ||||||
...snapshotMetadata.healthy, | ||||||
...snapshotMetadata.deferred, | ||||||
...snapshotMetadata.norewrite, | ||||||
...await createServerEntryPointBundle(buildAppDir), | ||||||
] | ||||||
|
||||||
// 2. Gather the dependencies that could potentially be removed from the binary due to being in the snapshot | ||||||
const potentiallyRemovedDependencies = [...snapshotMetadata.healthy, ...snapshotMetadata.deferred, ...snapshotMetadata.norewrite] | ||||||
console.log(`potentially removing ${potentiallyRemovedDependencies.length} dependencies`) | ||||||
|
||||||
// 3. Remove all dependencies that are in the snapshot but not in the list of kept dependencies from the binary | ||||||
await Promise.all(potentiallyRemovedDependencies.map(async (dependency) => { | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,21 @@ | ||||||
const Module = require('module') | ||||||
const path = require('path') | ||||||
|
||||||
process.env.CYPRESS_INTERNAL_ENV = process.env.CYPRESS_INTERNAL_ENV || 'production' | ||||||
try { | ||||||
require('./node_modules/bytenode/lib/index.js') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is index.js not the default export?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm actually trying to purposefully reference the exact file here. I don't anyone to be able to use any kind of module loading weirdness to reference a different file (may not be possible, but just in case). |
||||||
const filename = path.join(__dirname, 'packages', 'server', 'index.jsc') | ||||||
const dirname = path.dirname(filename) | ||||||
|
||||||
Module._extensions['.jsc']({ | ||||||
require: module.require, | ||||||
id: filename, | ||||||
filename, | ||||||
loaded: false, | ||||||
path: dirname, | ||||||
paths: Module._nodeModulePaths(dirname), | ||||||
}, filename) | ||||||
} catch (error) { | ||||||
BlueWinds marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
console.error(error) | ||||||
process.exit(1) | ||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing a lot in the binary build now.
xlarge
is needed to not run out of memory.