feat: various v8 snapshot improvements #24909
Conversation
|
Thanks for taking the time to open a PR!
|
|
|
||
| hookRequire(true) | ||
| } | ||
| hookRequire({ forceTypeScript: true }) |
There was a problem hiding this comment.
how do we know to force typescript?
There was a problem hiding this comment.
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.
scripts/after-pack-hook.js
Outdated
| 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) }) |
There was a problem hiding this comment.
| await setupV8Snapshots({ cypressAppPath: params.appOutDir, integrityCheckSource: getIntegrityCheckSource(outputFolder) }) | |
| await setupV8Snapshots({ | |
| cypressAppPath: params.appOutDir, integrityCheckSource: | |
| getIntegrityCheckSource(outputFolder), | |
| }) |
There was a problem hiding this comment.
Updated: 5c1f981 (#24909)
| const cleanup = async (buildAppDir) => { | ||
| 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.
Should this be:
| const entryPoints = [path.join(unixBuildAppDir, 'packages/server/index.js')] | |
| const entryPoints = [unixBuildAppDir, 'packages', 'server' , 'index.js'].join(path.posix.sep) |
There was a problem hiding this comment.
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:
and
| electron: true, | ||
| }) | ||
|
|
||
| return [...Object.keys(esbuildResult.metafile.inputs)].map((input) => `./${input}`) |
There was a problem hiding this comment.
Should this be a hard-coded separator?
There was a problem hiding this comment.
Yes, the way these files are read in assumes "require" syntax not platform specific syntax.
scripts/binary/binary-cleanup.js
Outdated
| // 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.
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.
I like this idea. Will work on it.
There was a problem hiding this comment.
Updated: 5c1f981 (#24909)
|
|
||
| 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.
is index.js not the default export?
| require('./node_modules/bytenode/lib/index.js') | |
| require('bytenode/lib/index.js') |
There was a problem hiding this comment.
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).
| // eslint-disable-next-line no-unused-vars | ||
| const stackIntegrityCheck = function stackIntegrityCheck (options) { | ||
| const originalStackTraceLimit = OrigError.stackTraceLimit | ||
| const originalStackTrace = OrigError.prepareStackTrace |
There was a problem hiding this comment.
| const originalStackTrace = OrigError.prepareStackTrace | |
| const originalPrepareStackTrace = OrigError.prepareStackTrace |
There was a problem hiding this comment.
Updated: 5c1f981 (#24909)
| const originalStackTraceLimit = OrigError.stackTraceLimit | ||
| const originalStackTrace = OrigError.prepareStackTrace |
There was a problem hiding this comment.
Do we need to create each time or can we make these global?
There was a problem hiding this comment.
This function is really only called once. I like the idea of storing this off just in time since we are replacing them immediately after. The other global concepts (Error, captureStackTrace, etc.) are ones that we don't do a replacement dance with.
| return stack | ||
| } | ||
|
|
||
| const tempError = new OrigError |
There was a problem hiding this comment.
Is is possible clone the class instance so we don't need to reset the overrides?
There was a problem hiding this comment.
I think there's some alternative things we can do here, but I think I like this pattern best.
| const toString = Function.prototype.toString | ||
| const callFn = Function.call | ||
|
|
||
| // eslint-disable-next-line no-unused-vars |
There was a problem hiding this comment.
is this disable check needed? we are using it below without conditional checks
There was a problem hiding this comment.
Ah yeah. This can be removed.
There was a problem hiding this comment.
Removed: 5c1f981 (#24909)
| } | ||
|
|
||
| for (let index = 0; index < options.stackToMatch.length; index++) { | ||
| const expectedFunctionName = options.stackToMatch[index].functionName |
There was a problem hiding this comment.
| const expectedFunctionName = options.stackToMatch[index].functionName | |
| const { expectedFunctionName, expectedFileName} = options.stackToMatch[index] |
There was a problem hiding this comment.
Updated: 5c1f981 (#24909)
| const expectedFileName = options.stackToMatch[index].fileName | ||
| const actualFileName = stack[index].getFileName() | ||
|
|
||
| if (expectedFunctionName && actualFunctionName !== expectedFunctionName) { |
There was a problem hiding this comment.
should we throw if expectedFunctionName or expectedFunctionName are undefined? can we get in this situation?
There was a problem hiding this comment.
looking below looks like expectedFunctionName depends on the file
There was a problem hiding this comment.
We control what's passed in here so we shouldn't be in that situation.
| } | ||
|
|
||
| function validateFs (fs) { | ||
| if (toString.call(fs.readFileSync) !== `function(t,r){const n=splitPath(t);if(!n.isAsar)return g.apply(this,arguments);const{asarPath:i,filePath:a}=n,o=getOrCreateArchive(i);if(!o)throw createError("INVALID_ARCHIVE",{asarPath:i});const c=o.getFileInfo(a);if(!c)throw createError("NOT_FOUND",{asarPath:i,filePath:a});if(0===c.size)return r?"":s.Buffer.alloc(0);if(c.unpacked){const t=o.copyFileOut(a);return e.readFileSync(t,r)}if(r){if("string"==typeof r)r={encoding:r};else if("object"!=typeof r)throw new TypeError("Bad arguments")}else r={encoding:null};const{encoding:f}=r,l=s.Buffer.alloc(c.size),u=o.getFdAndValidateIntegrityLater();if(!(u>=0))throw createError("NOT_FOUND",{asarPath:i,filePath:a});return logASARAccess(i,a,c.offset),e.readSync(u,l,0,c.size,c.offset),validateBufferIntegrity(l,c.integrity),f?l.toString(f):l}`) { |
There was a problem hiding this comment.
risk this will ever be different given it is internal node code?
There was a problem hiding this comment.
This is actually electron code not node code and there's not really a good way for me to get at it at package time. I think we are safe to update this as we update electron (if we need to). I'll add a comment for posterity though.
There was a problem hiding this comment.
Clarified: 5c1f981 (#24909)
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
Co-authored-by: Emily Rohrbough <emilyrohrbough@users.noreply.github.com>
User facing changelog
Further improved the startup time and decreased the size of the binary by using
bytenodeto pre-compile the entry point.Additional details
bytenodeas the mechanism to precompile the entry point.bytenodeandv8already provide some level of integrity checking on the compiled code it generates, but we also added additional checks to protect the binary even further.Steps to test
The best way to test this is with the downloaded binary attached to the last commit of this PR.
How has the user experience changed?
n/a
PR Tasks
cypress-documentation?type definitions?