N8N-2827 speed up nodes base #3110
Conversation
|
|
Joffcom
added
core
alexgrozav
force-pushed
the
N8N-2827-speed-up-nodes-base
branch
3 times, most recently
from
04a7457 to
73da27d
Compare
alexgrozav
force-pushed
the
N8N-2827-speed-up-nodes-base
branch
from
bc44c1a to
1c9b8c9
Compare
alexgrozav
force-pushed
the
N8N-2827-speed-up-nodes-base
branch
from
b77c03e to
b9025f2
Compare
There was a problem hiding this comment.
Sorry for the delay! Re-review:
- n8n fails to initialize - unable to load some esbuilt nodes:
git reset --hard; git clean -dffx
lerna bootstrap --hoist; npm run build
npm run start
> [email protected] start
> run-script-os
> [email protected] start:default
> cd packages/cli/bin && ./n8n
Initializing n8n process
› Error: There was an error: Cannot find module './v1/actions/router'
› Require stack:
› - /Users/ivov/Development/n8n/packages/nodes-base/dist/nodes/BambooHr/BambooHr.node.js
› - /Users/ivov/Development/n8n/packages/cli/dist/src/LoadNodesAndCredentials.js
› - /Users/ivov/Development/n8n/packages/cli/dist/src/index.js
› - /Users/ivov/Development/n8n/packages/cli/dist/commands/execute.js
› - /Users/ivov/Development/n8n/node_modules/@oclif/config/lib/plugin.js
› - /Users/ivov/Development/n8n/node_modules/@oclif/config/lib/config.js
› - /Users/ivov/Development/n8n/node_modules/@oclif/config/lib/index.js
› - /Users/ivov/Development/n8n/node_modules/@oclif/command/lib/command.js
› - /Users/ivov/Development/n8n/node_modules/@oclif/command/lib/index.js
› - /Users/ivov/Development/n8n/packages/cli/bin/n8n
Removing Bamboo HR triggers same error for Cisco Webex and then Elasticsearch.
.d.tsare still being included, becoming.d.jsin /dist. The glob inbuild.mjsis not excluding them:console.log(tsFiles.filter(f => f.endsWith('.d.ts')));- The existing TS build process emits CJS with the use strict directive; esbuild is not doing this, but supports a workaround.
- Switching to a non-English locale causes the build to fail:
export N8N_DEFAULT_LOCALE=de
cd packages/nodes-base; npm run build
node:events:368
throw er; // Unhandled 'error' event
^
Error: ENOENT: no such file or directory, open '/Users/ivov/Development/n8n/packages/nodes-base/dist/nodes/headers.js'
Emitted 'error' event on Domain instance at:
at emit (node:internal/process/promises:134:35)
at processPromiseRejections (node:internal/process/promises:242:25)
at processTicksAndRejections (node:internal/process/task_queues:97:32) {
errno: -2,
code: 'ENOENT',
syscall: 'open',
path: '/Users/ivov/Development/n8n/packages/nodes-base/dist/nodes/headers.js'
}
ERROR: "build:assets" exited with 1.
This issue is absent from master. The build stages are now parallel, so I expect the attempt to write to the headers path finishes first, failing to find the dist dir.
Edit: 4.5. Should we restore type-checking to the build process with --noEmit?
Minor details
'\n[esbuild] Watching for changes...'is shown both for watch and for build, instead of just for watch. You can use thewatch.onRebuildarg, refer to the second watch mode example:
require('esbuild').build({
entryPoints: ['app.js'],
outfile: 'out.js',
bundle: true,
watch: {
onRebuild(error, result) {
if (error) console.error('watch build failed:', error)
else console.log('watch build succeeded:', result)
},
},
}).then(result => {
console.log('watching...')
})- Instead of removing the existing watch script reference in
package.json, we should keep it aswatch:oldjust like the old build script. In a few weeks if there are no issues, we can remove both. - After deletion of
watch.mjs,shelljsis now only used forrm -rf. We already haverimraf, soshelljsmight not be needed. - The only JSON files in /nodes-base/nodes and /nodes-base/credentials are codex files
.node.jsonand potentially translation files. Both kinds are required cross-package from /cli, i.e. they are not directly imported inside /nodes-base, so I expect thatnodes/**/*.jsonandcredentials/translations/**/*.jsonshould be removable fromtsconfig.jsonwithout import issues.
find ./packages/nodes-base -type f -name \*.json ! -name \*.node.json
- (Nitpick) Naming of
tsconfig.build.json- the child only takes care of building /src, but both parent and child are responsible for the entire build, which makes the.build.infix for /src potentially confusing. This will become moot once we move src into /cli and skip declaration generation altogether. - (Out of scope, tech debt.) We should eventually explore the esbuild minify and tree-shaking flags.
🚧 Updated dependencies and cleaned up. ⚡ Building nodes-base via esbuild. 🐛 Added copying json files to gulp task. ♻️ Fixed EditImage esModuleInterop warning and PostMark empty body check warning. ✨ Updated npm-run-all to run in parallel ♻️ Refactored pg namespace import. ♻️ Switched to esbuild watch from chokidar. ♻️ Removed unnecessary params from tsconfig.build.json.
alexgrozav
force-pushed
the
N8N-2827-speed-up-nodes-base
branch
from
b9025f2 to
f37a6bc
Compare
The issue seems to have been
Fixed. Fast glob supports negation glob.
Added
Fixed. Ensured folder creation upon build. Extracted cleanup phase to start before parallel execution (duh! 😰).
Added proper build message for build mode and watch mode.
Removed
Removed irrelevant files from tsconfig.json.
Renamed it to
Definitely! I'll create a task for it after this one gets merged. |
|
Thanks for addressing my comments 👍
|
Looks like the
Not much I can do about this, unfortunately.
|
There was a problem hiding this comment.
Thank you for the changes. Some final comments:
-
It is unusual that we only need to build
/nodes-basein isolation./nodes-baseis almost always built together with all other packages viadevin the rootpackage.json, i.e.lerna exec npm run dev --parallel, which will runnpm run buildin/nodes-base. This means thatnpm run build:fastis unlikely to be used in day-to-day work. Should we add a root command for fast builds? -
skipLibCheckwill also skip checking of declaration files in the project itself, which is relevant because many nodes contain custom.d.tsfiles. The best of both worlds would be enabling this flag for the speed boost, but also converting all.d.tsfiles to.tsto ensure they're type-checked. Context: 1 2 3 4 Maybe we should consider delaying this flag until we can take the time to convert them. -
build.mjsis throwing off ESLint:
Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser.
The file does not match your project config: packages/nodes-base/scripts/build.mjs.
The extension for the file (.mjs) is non-standard. You should add "parserOptions.extraFileExtensions" to your config.
From a quick check, mjs needs to be added to extraFileExtensions or converted to mts which is supported by default, or else we could disregard mjs in .eslintignore.
-
build:typecheckis now inlining multiple flags to avoid using--project. These flags are a mix of settings already present intsconfig.jsone.g.moduleand of settings absent fromtsconfig.json, e.g.downlevelIterationandallowSyntheticDefaultImports. I am reluctant to introduce duplication and add new compiler flags that were not needed before and that required changes to the source as in Magento and Action Network, for a 2s gain. Commenting mostly to bring up the concern, but up to you. -
Why switch
moduleandtargetto ESNext? From the TS docs:The special ESNext value refers to the highest version your version of TypeScript supports. This setting should be used with caution, since it doesn’t mean the same thing between different TypeScript versions and can make upgrades less predictable. -
fast-globis listed twice innodes-base/package.json, as regular dependency and as dev dependency. -
"test/**/*"is being included and"**/*.spec.ts"is being excluded but all tests in/nodes-baseare JS.
|
Update:
|
There was a problem hiding this comment.
Nice idea to bring in Turborepo :)
- Can you please run these? Before making any changes from the review.
git reset --hard; git clean -dffx
lerna bootstrap --hoist
time npm run build
// make a change in a file
time npm run build
system_profiler SPHardwareDataType
I get 2m 10s (first build) and 33s (cached build) on:
Model Name: MacBook Pro
Model Identifier: MacBookPro17,1
Chip: Apple M1
Total Number of Cores: 8 (4 performance and 4 efficiency)
Memory: 16 GB
Comparing against my baseline benchmarks without esbuild and without turbo, only the cached build shows gains, but the first build is identical - this is surprising, as switching to esbuild in nodes-base should give us some gain.
- We are removing
"nodes/**/*.json"frompackages/nodes-base/tsconfig.json, so with this new setup we no longer have codex files in dist. Therefore we should confirm that no external services rely on codex files in dist. For example,Supported ActionsandBasic Operationsare likely reading from codex files. Bringing this up mostly as I do not know offhand whether they would be affected - if they are reading from "untranspiled" codex files, then no issue.
packages/nodes-base/package.json
Outdated
| "build": "rimraf dist && npm-run-all --parallel build:esbuild build:gulp build:tsc", | ||
| "build:esbuild": "node scripts/build.mjs", | ||
| "build:gulp": "gulp build:assets build:translations", | ||
| "build:tsc": "tsc --project tsconfig.json --emitDeclarationOnly", |
There was a problem hiding this comment.
- Do we need to specify the path to tsconfig with
--project? --emitDeclarationOnlyis no longer be needed I think? Also should this not be--noEmit?
There was a problem hiding this comment.
- No need to specify project.
- Added d.ts emitting since it doesn't impact build speed.
| const tsFiles = (await glob([path.resolve(rootDir, '{credentials,nodes,src}/**/*.ts')])) | ||
| .filter((file) => !file.endsWith('.d.ts')); |
There was a problem hiding this comment.
srcno longer exists.- This runs very frequently and over a large number of files, so let's optimize. One idea is to recurse and check during traversal. What do you think?
function walk(dir, test, collection = []) {
fs.readdirSync(dir).forEach((entry) => {
const entryPath = path.resolve(dir, entry);
if (fs.lstatSync(entryPath).isDirectory()) {
walk(entryPath, test, collection);
}
if (test(entry)) collection.push(entryPath);
});
return collection;
}
const isTsFile = (filepath) => /(?<!\.d)\.ts$/.test(filepath);
const tsFiles = ['nodes', 'credentials'].flatMap((dir) => walk(dir, isTsFile));There was a problem hiding this comment.
- Removed
src - That's essentially what the glob does as well, and it runs only once. The overhead is
tsc, unfortunately. Thebuild.mjsscript runs in a few hundred milliseconds.
There was a problem hiding this comment.
- But after glob there is filter so double iteration?
walkis intended to avoid double iteration as it includesfilterastest. Let me know if I'm misunderstanding.
| import type { Workflow } from './Workflow'; | ||
| import type { WorkflowHooks } from './WorkflowHooks'; | ||
| import type { WorkflowActivationError } from './WorkflowActivationError'; | ||
| import type { WorkflowOperationError } from './WorkflowErrors'; | ||
| import type { NodeApiError, NodeOperationError } from './NodeErrors'; |
There was a problem hiding this comment.
Since you are touching these, let's also cover L6 to L9.
There was a problem hiding this comment.
Updated to import type.
| @@ -1,25 +1,29 @@ | |||
| import { INodeType, INodeTypeBaseDescription, INodeVersionedType } from 'n8n-workflow'; | |||
| import { INodeType, INodeTypeBaseDescription, INodeVersionedType } from './Interfaces'; | |||
There was a problem hiding this comment.
| import { INodeType, INodeTypeBaseDescription, INodeVersionedType } from './Interfaces'; | |
| import type { INodeType, INodeTypeBaseDescription, INodeVersionedType } from './Interfaces'; |
| description: INodeTypeBaseDescription; | ||
|
|
||
| constructor(nodeVersions: INodeVersionedType['nodeVersions'], description: INodeTypeBaseDescription) { | ||
| constructor( |
There was a problem hiding this comment.
One concern with relocating this class purely for build performance reasons is that we are not keeping related functionality together, since NodeVersionedType fits better in nodes-base than in workflow.
There was a problem hiding this comment.
I've had the same concern. But after giving it some thought, the NodeVersionedType.ts file doesn't look like something that's likely to change too often, and it directly implements an interface declared in workflow, so the location felt right.
In my opinion the build speed benefit will be worth it in the long run. What do you think?
| @@ -0,0 +1,41 @@ | |||
| { | |||
| "$schema": "https://turborepo.org/schema.json", | |||
There was a problem hiding this comment.
General question: If ^build makes any package's build depend on its dependencies' build having completed first, as explained here, then why do we need to specify our chain of dependencies by hand with the keys below? And why does the specified list of dependencies in turbo.json not reflect the dependency lists in package.json in our various packages?
design-systemdoes not depend on any package, butturbo.jsonlistscoreas a dependency.editor-uidepends ondesign-systemandworkflow, butturbo.jsonlistscoreas a dependency.clidepends onworkflow,coreandeditor-ui, butturbo.jsonagain listscore.
(Never used Turborepo before, there is probably a reason, mostly curious.)
Smaller comments:
- Turborepo always needs
outputsto be specified and is unable to find this intsconfig.json? format,lintandlintfixare not included inturbo.json.testis included, but it is still set to run with lerna in the rootpackage.json.- We do not use
tsx. fullis the default output mode and so does not need to be specified.- Re:
inputs, not all code relevant to tests is in/srcand/test, e.g./cli/commands,/cli/config,/nodes-base/nodes, etc. "n8n-editor-ui#build"When usingvue-cli-service build, I notice it reportsUsing 1 worker with 2048MB memory limit. Sinceeditor-uiis one of the slowest building packages, can we experiment with adding workers and raising the memory limit?
There was a problem hiding this comment.
Looks like there's no real benefit in defining build dependencies explicitly. When left to use the dependency graph, turborepo does a great job by itself.
- You're right. It's able to infer the build outputs.
- I have not looked into
testformat,lintandlintfixyet. Will do now. - Good point. Removed.
- Removed
outputMode. - Removed
inputs - Increased memory limit, but doesn't impact build speed unfortunately. Webpack / Vue CLI is still a bottleneck until we migrate to Vite.js.
There was a problem hiding this comment.
Converted remaining lerna run scripts.
fix: Added turbo-linux-64 to CI step. x
alexgrozav
force-pushed
the
N8N-2827-speed-up-nodes-base
branch
from
788ef78 to
92e2f8e
Compare
Issue
Building thousands of files in the nodes-base package is slow. Attempt switching to vite or esbuild.
Findings
Switching to Vite.js
Vite.js is focused on creating application bundles and packages. This means that there should be only a few entry points. The
nodes-basepackage has hundreds of entry points that need to be compiled individually. Even though Vite is super fast, it's not what it is built for.Vite.js build time
Switching to esbuild
Even after switching to esbuild, I was only able to shave off a few seconds from the build because the
.d.tsstill need to be generated usingtsc. Esbuild is fast, but it simply ignores typings when compiling evanw/esbuild#95 (comment).Proposed solution
Esbuild is able to compile the whole project in under
1swithout type definitions. The only typings used outside of development are found in thesrcfolder. Instead of generating typescript definition files for each node, we can generate them only for the files inside the src folder.This dramatically reduces build time.
Old build time
New build time
If still needed, we could add a script for generating the
d.tsfor nodes when creating a new release.Alternatively, could let the
buildcommand build alld.tsfiles and then have abuild:fastto rebuild.tsusingesbuild, and copy assets only.What do you think?