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

feat: various v8 snapshot improvements #24909

Merged
merged 60 commits into from Dec 3, 2022
Merged
Show file tree
Hide file tree
Changes from 55 commits
Commits
Show all changes
60 commits
Select commit Hold shift + click to select a range
3f3e308
fix: v8 improvements - run ci
ryanthemanuel Nov 24, 2022
7d65a96
fix: v8 improvements - run ci
ryanthemanuel Nov 24, 2022
fcfa789
lock things down even more
ryanthemanuel Nov 27, 2022
90c96c6
Fix build
ryanthemanuel Nov 27, 2022
5493177
fix build
ryanthemanuel Nov 28, 2022
474b3a4
add memory for packaging
ryanthemanuel Nov 28, 2022
e6cde79
additional tweaking
ryanthemanuel Nov 28, 2022
ac5ebdc
debug logging
ryanthemanuel Nov 28, 2022
25b870c
debugging
ryanthemanuel Nov 28, 2022
968e726
code cleanup
ryanthemanuel Nov 28, 2022
51426dd
fix build
ryanthemanuel Nov 28, 2022
c071402
use api rather than command line
ryanthemanuel Nov 28, 2022
8ab76e6
bump memory
ryanthemanuel Nov 28, 2022
b4e2261
fix linux arm64 issue
ryanthemanuel Nov 28, 2022
df39189
fix build
ryanthemanuel Nov 28, 2022
239a953
harden some of the logic
ryanthemanuel Nov 30, 2022
b4f8119
minor refactoring
ryanthemanuel Nov 30, 2022
b77cf7c
more refactoring
ryanthemanuel Nov 30, 2022
e2dd80f
Merge branch 'develop' into ryanm/fix/v8-improvements
ryanthemanuel Nov 30, 2022
dc0332a
Update workflows.yml
ryanthemanuel Nov 30, 2022
6b5185c
run ci
ryanthemanuel Nov 30, 2022
5e6f47d
run ci
ryanthemanuel Nov 30, 2022
42f8935
Update config.yml
ryanthemanuel Nov 30, 2022
0c2dfae
run ci
ryanthemanuel Nov 30, 2022
d48caa8
limit integrity check to when there are snapshots
ryanthemanuel Nov 30, 2022
528d91b
fix build
ryanthemanuel Dec 1, 2022
9a4157e
fix build
ryanthemanuel Dec 1, 2022
35678ef
fix build
ryanthemanuel Dec 1, 2022
cb7e3b2
attempt to fix tests
ryanthemanuel Dec 1, 2022
1a4c521
attempt to fix tests
ryanthemanuel Dec 1, 2022
1c1ce89
get more debugging info
ryanthemanuel Dec 1, 2022
76a18b6
get more debugging info
ryanthemanuel Dec 1, 2022
1720cb8
update cache files
ryanthemanuel Dec 1, 2022
3ea52dc
update cache files
ryanthemanuel Dec 1, 2022
250e9f8
update cache files
ryanthemanuel Dec 1, 2022
9ba3016
Merge branch 'develop' into ryanm/fix/v8-improvements
ryanthemanuel Dec 1, 2022
35c8b6f
update yarn.lock
ryanthemanuel Dec 1, 2022
401fbdc
fix build
ryanthemanuel Dec 1, 2022
d3c832a
fix windows
ryanthemanuel Dec 1, 2022
eb0dcfe
update cache
ryanthemanuel Dec 1, 2022
f27b687
Update scripts/binary/binary-cleanup.js
ryanthemanuel Dec 1, 2022
b199681
Update scripts/binary/binary-cleanup.js
ryanthemanuel Dec 1, 2022
bd533d3
Update scripts/binary/binary-cleanup.js
ryanthemanuel Dec 1, 2022
8d2598a
update caches
ryanthemanuel Dec 1, 2022
39d02d6
slight refactor
ryanthemanuel Dec 1, 2022
ce8c74a
Merge branch 'develop' into ryanm/fix/v8-improvements
ryanthemanuel Dec 1, 2022
fe8b6cd
pr comments
ryanthemanuel Dec 1, 2022
bb1390e
Update cache-version.txt
ryanthemanuel Dec 1, 2022
aa6b359
Merge branch 'develop' into ryanm/fix/v8-improvements
ryanthemanuel Dec 2, 2022
4187ff6
Update scripts/binary/binary-integrity-check-source.js
ryanthemanuel Dec 2, 2022
ab63313
Update scripts/binary/binary-integrity-check-source.js
ryanthemanuel Dec 2, 2022
6751287
Update scripts/binary/binary-integrity-check-source.js
ryanthemanuel Dec 2, 2022
bebcd8a
Update scripts/binary/binary-integrity-check-source.js
ryanthemanuel Dec 2, 2022
30660b0
Update scripts/binary/binary-integrity-check-source.js
ryanthemanuel Dec 2, 2022
5c1f981
pr comments
ryanthemanuel Dec 2, 2022
adadd79
further comments
ryanthemanuel Dec 2, 2022
5f3a735
update messaging on failures
ryanthemanuel Dec 2, 2022
12a5753
Merge branch 'develop' into ryanm/fix/v8-improvements
ryanthemanuel Dec 2, 2022
5442930
Merge branch 'develop' into ryanm/fix/v8-improvements
ryanthemanuel Dec 3, 2022
4e8d8e3
update messaging on failures
ryanthemanuel Dec 3, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/cache-version.txt
@@ -1,3 +1,3 @@
# Bump this version to force CI to re-create the cache from scratch.

10-31-22
12-01-22
12 changes: 6 additions & 6 deletions .circleci/workflows.yml
Expand Up @@ -27,7 +27,7 @@ mainBuildFilters: &mainBuildFilters
branches:
only:
- develop
- 'feature/run-all-specs'
- 'ryanm/fix/v8-improvements'

# usually we don't build Mac app - it takes a long time
# but sometimes we want to really confirm we are doing the right thing
Expand All @@ -36,7 +36,7 @@ macWorkflowFilters: &darwin-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'feature/run-all-specs', << pipeline.git.branch >> ]
- equal: [ 'ryanm/fix/v8-improvements', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand All @@ -45,7 +45,7 @@ linuxArm64WorkflowFilters: &linux-arm64-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'feature/run-all-specs', << pipeline.git.branch >> ]
- equal: [ 'ryanm/fix/v8-improvements', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand All @@ -63,7 +63,7 @@ windowsWorkflowFilters: &windows-workflow-filters
when:
or:
- equal: [ develop, << pipeline.git.branch >> ]
- equal: [ 'astone123/fix-windows-lint', << pipeline.git.branch >> ]
- equal: [ 'ryanm/fix/v8-improvements', << pipeline.git.branch >> ]
- matches:
pattern: "-release$"
value: << pipeline.git.branch >>
Expand Down Expand Up @@ -130,7 +130,7 @@ commands:
- run:
name: Check current branch to persist artifacts
command: |
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "feature/run-all-specs" ]]; then
if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "ryanm/fix/v8-improvements" ]]; then
echo "Not uploading artifacts or posting install comment for this branch."
circleci-agent step halt
fi
Expand Down Expand Up @@ -1989,7 +1989,7 @@ jobs:
<<: *defaultsParameters
resource_class:
type: string
default: large
default: xlarge
Copy link
Collaborator Author

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.

resource_class: << parameters.resource_class >>
steps:
- restore_cached_workspace
Expand Down
4 changes: 3 additions & 1 deletion package.json
Expand Up @@ -4,7 +4,7 @@
"description": "Cypress is a next generation front end testing tool built for the modern web",
"private": true,
"scripts": {
"binary-build": "node ./scripts/binary.js build",
"binary-build": "cross-env NODE_OPTIONS=--max_old_space_size=8192 node ./scripts/binary.js build",
"binary-deploy": "node ./scripts/binary.js deploy",
"binary-deploy-linux": "./scripts/build-linux-binary.sh",
"binary-ensure": "node ./scripts/binary.js ensure",
Expand Down Expand Up @@ -70,6 +70,7 @@
"prepare": "husky install"
},
"dependencies": {
"bytenode": "1.3.7",
"nvm": "0.0.4"
},
"devDependencies": {
Expand Down Expand Up @@ -140,6 +141,7 @@
"commander": "6.2.1",
"common-tags": "1.8.0",
"conventional-recommended-bump": "6.1.0",
"cross-env": "7.0.3",
"debug": "^4.3.2",
"dedent": "^0.7.0",
"del": "3.0.0",
Expand Down
9 changes: 2 additions & 7 deletions packages/rewriter/script/worker-shim.js
Expand Up @@ -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 })
Copy link
Member

Choose a reason for hiding this comment

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

how do we know to force typescript?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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')
6 changes: 3 additions & 3 deletions packages/server/hook-require.js
Expand Up @@ -9,7 +9,7 @@ function runWithSnapshot (forceTypeScript) {
const { snapshotRequire } = require('@packages/v8-snapshot-require')
const projectBaseDir = process.env.PROJECT_BASE_DIR

const supportTS = forceTypeScript || typeof global.snapshotResult === 'undefined' || global.supportTypeScript
const supportTS = forceTypeScript || typeof global.getSnapshotResult === 'undefined' || global.supportTypeScript

snapshotRequire(projectBaseDir, {
diagnosticsEnabled: isDev,
Expand All @@ -30,8 +30,8 @@ function runWithSnapshot (forceTypeScript) {
})
}

const hookRequire = (forceTypeScript) => {
if (['1', 'true'].includes(process.env.DISABLE_SNAPSHOT_REQUIRE) || typeof snapshotResult === 'undefined') {
const hookRequire = ({ forceTypeScript }) => {
if (['1', 'true'].includes(process.env.DISABLE_SNAPSHOT_REQUIRE) || typeof getSnapshotResult === 'undefined') {
require('@packages/ts/register')
} else {
runWithSnapshot(forceTypeScript)
Expand Down
21 changes: 11 additions & 10 deletions packages/server/index.js
@@ -1,18 +1,19 @@
const { initializeStartTime } = require('./lib/util/performance_benchmark')

const run = async () => {
initializeStartTime()
const startCypress = async () => {
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('./start-cypress')
} catch (error) {
// eslint-disable-next-line no-console
console.error(error)
process.exit(1)
}

await require('./server-entry')
}

module.exports = run()
module.exports = startCypress()
5 changes: 3 additions & 2 deletions packages/server/package.json
Expand Up @@ -194,10 +194,11 @@
},
"files": [
"config",
"hook-require.js",
"lib",
"patches",
"server-entry.js",
"hook-require.js"
"start-cypress.js",
"v8-snapshot-entry.js"
],
"types": "index.d.ts",
"productName": "Cypress",
Expand Down
File renamed without changes.
1 change: 1 addition & 0 deletions packages/server/v8-snapshot-entry.js
@@ -0,0 +1 @@
require('./start-cypress')
4 changes: 2 additions & 2 deletions packages/ts/registerDir.js
Expand Up @@ -8,8 +8,8 @@ const path = require('path')
// build has been done correctly
module.exports = function (scopeDir) {
// Only set up ts-node if we're not using the snapshot
// @ts-ignore snapshotResult is a global defined in the v8 snapshot
if (['1', 'true'].includes(process.env.DISABLE_SNAPSHOT_REQUIRE) || typeof snapshotResult === 'undefined') {
// @ts-ignore getSnapshotResult is a global defined in the v8 snapshot
if (['1', 'true'].includes(process.env.DISABLE_SNAPSHOT_REQUIRE) || typeof getSnapshotResult === 'undefined') {
try {
// Prevent double-compiling if we're testing the app and already have ts-node hook installed
// TODO(tim): e2e testing does not like this, I guess b/c it's currently using the tsconfig
Expand Down
17 changes: 7 additions & 10 deletions packages/v8-snapshot-require/src/snapshot-require.ts
Expand Up @@ -181,8 +181,8 @@ export function snapshotRequire (
// 1. Assign snapshot which is a global if it was embedded
const sr: Snapshot =
opts.snapshotOverride ||
// @ts-ignore global snapshotResult
(typeof snapshotResult !== 'undefined' ? snapshotResult : undefined)
// @ts-ignore global getSnapshotResult
(typeof getSnapshotResult !== 'undefined' ? getSnapshotResult() : undefined)

// If we have no snapshot we don't need to hook anything
if (sr != null || alwaysHook) {
Expand Down Expand Up @@ -239,10 +239,7 @@ export function snapshotRequire (
moduleNeedsReload,
})

// @ts-ignore global snapshotResult
// 8. Ensure that the user passed the project base dir since the loader
// cannot resolve modules without it
if (typeof snapshotResult !== 'undefined') {
if (typeof sr !== 'undefined') {
const projectBaseDir = process.env.PROJECT_BASE_DIR

if (projectBaseDir == null) {
Expand Down Expand Up @@ -290,8 +287,8 @@ export function snapshotRequire (

// 11. Inject those globals

// @ts-ignore global snapshotResult
snapshotResult.setGlobals(
// @ts-ignore setGlobals is a function on global sr
sr.setGlobals(
global,
checked_process,
checked_window,
Expand All @@ -305,8 +302,8 @@ export function snapshotRequire (

// @ts-ignore private module var
require.cache = Module._cache
// @ts-ignore global snapshotResult
snapshotResult.customRequire.cache = require.cache
// @ts-ignore customRequire is a property of global sr
sr.customRequire.cache = require.cache

// 12. Add some 'magic' functions that we can use from inside the
// snapshot in order to integrate module loading
Expand Down
13 changes: 10 additions & 3 deletions scripts/after-pack-hook.js
Expand Up @@ -6,7 +6,8 @@ const os = require('os')
const path = require('path')
const { setupV8Snapshots } = require('@tooling/v8-snapshot')
const { flipFuses, FuseVersion, FuseV1Options } = require('@electron/fuses')
const { cleanup } = require('./binary/binary-cleanup')
const { buildEntryPointAndCleanup } = require('./binary/binary-cleanup')
const { getIntegrityCheckSource, getBinaryEntryPointSource } = require('./binary/binary-sources')

module.exports = async function (params) {
console.log('****************************')
Expand Down Expand Up @@ -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()],
{
Expand All @@ -63,7 +66,11 @@ module.exports = async function (params) {
},
)

await setupV8Snapshots(params.appOutDir)
await cleanup(outputFolder)
// Build out the entry point and clean up prior to setting up v8 snapshots so that the state of the binary is correct
await buildEntryPointAndCleanup(outputFolder)
await setupV8Snapshots({
cypressAppPath: params.appOutDir,
integrityCheckSource: getIntegrityCheckSource(outputFolder),
})
}
}
74 changes: 59 additions & 15 deletions scripts/binary/binary-cleanup.js
Expand Up @@ -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)

Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -81,7 +80,7 @@ const getDependencyPathsToKeep = async (buildAppDir) => {
absWorkingDir: unixBuildAppDir,
external: [
'./transpile-ts',
'./server-entry',
'./start-cypress',
'fsevents',
'pnpapi',
'@swc/core',
Expand Down Expand Up @@ -111,18 +110,63 @@ const getDependencyPathsToKeep = async (buildAppDir) => {
})
}

return [...Object.keys(esbuildResult.metafile.inputs), ...entryPoints]
return [...Object.keys(esbuildResult.metafile.inputs), ...entryPoints, 'package.json']
}

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 createServerEntryPointBundle = async (buildAppDir) => {
const unixBuildAppDir = buildAppDir.split(path.sep).join(path.posix.sep)
const entryPoints = [path.join(unixBuildAppDir, 'packages/server/index.js')]
Copy link
Member

Choose a reason for hiding this comment

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

Should this be:

Suggested change
const entryPoints = [path.join(unixBuildAppDir, 'packages/server/index.js')]
const entryPoints = [unixBuildAppDir, 'packages', 'server' , 'index.js'].join(path.posix.sep)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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. path.posix.sep is something we already handle uniquely in the snapshot (since when we're generating the snapshot we don't have access to path or any other node internals), so I just decided to reuse what we had available.

See:

https://github.com/cypress-io/cypress/blob/develop/tooling/v8-snapshot/src/generator/blueprint.ts#L56

and

https://github.com/cypress-io/cypress/blob/develop/tooling/v8-snapshot/src/generator/blueprint.ts#L110

// Build the binary entry point ignoring anything that happens in start-cypress since that will be in the v8 snapshot
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',
'./start-cypress',
],
})

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,
})

// Convert these inputs to a relative file path. Note that these paths are posix paths.
return [...Object.keys(esbuildResult.metafile.inputs)].map((input) => `./${input}`)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a hard-coded separator?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 buildEntryPointAndCleanup = async (buildAppDir) => {
const [keptDependencies, serverEntryPointBundleDependencies] = await Promise.all([
// 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
getDependencyPathsToKeep(buildAppDir),
// 2. Create a bundle for the server entry point. This will be used to start the server in the binary. It returns the dependencies that are pulled in by this bundle that potentially can now be removed
createServerEntryPointBundle(buildAppDir),
])

// 3. 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,
...serverEntryPointBundleDependencies,
]

// 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
// 4. 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) => {
const typeScriptlessDependency = dependency.replace(/\.ts$/, '.js')

Expand All @@ -132,10 +176,10 @@ const cleanup = async (buildAppDir) => {
}
}))

// 4. Consolidate dependencies that are safe to consolidate (`lodash` and `bluebird`)
// 5. Consolidate dependencies that are safe to consolidate (`lodash` and `bluebird`)
await consolidateDeps({ projectBaseDir: buildAppDir })

// 5. Remove various unnecessary files from the binary to further clean things up. Likely, there is additional work that can be done here
// 6. Remove various unnecessary files from the binary to further clean things up. Likely, there is additional work that can be done here
await del([
// Remove test files
path.join(buildAppDir, '**', 'test'),
Expand Down Expand Up @@ -187,10 +231,10 @@ const cleanup = async (buildAppDir) => {
path.join(buildAppDir, '**', 'yarn.lock'),
], { force: true })

// 6. Remove any empty directories as a result of the rest of the cleanup
// 7. Remove any empty directories as a result of the rest of the cleanup
await removeEmptyDirectories(buildAppDir)
}

module.exports = {
cleanup,
buildEntryPointAndCleanup,
}