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(core): Lazy-load nodes and credentials to reduce baseline memory usage #4577

Merged
merged 42 commits into from
Nov 23, 2022

Conversation

netroy
Copy link
Member

@netroy netroy commented Nov 10, 2022

over 50% drop in baseline memory usage, by loading nodes and credentials as they are needed, instead of loading them all at startup.

image

@linear
Copy link

linear bot commented Nov 10, 2022

N8N-4532

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system labels Nov 10, 2022
@netroy netroy force-pushed the N8N-4532-lazy-loading branch 10 times, most recently from 1f9c7ed to 4e78c04 Compare November 15, 2022 16:26
Copy link
Contributor

@ivov ivov left a comment

Choose a reason for hiding this comment

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

  1. This breaks the dev flow for node builders in the team and in the community, because watch transpiles changes to dist but does not reflect them in the root dir cache. We have to keep the cache fresh or bypass it for watch.
  2. In /dist/known/credentials.json the mapping from credential type name to className and sourcePath looks inferrable. Do we need these two details to load a credential type? In /dist/known/nodes.json, the mapping from node type name to className looks inferrable but sourcePath becomes unpredictable because of grouping dirs like Aws, which btw I am not sure we benefit much from.

packages/nodes-base/scripts/generate-metadata.js Outdated Show resolved Hide resolved
packages/nodes-base/scripts/generate-metadata.js Outdated Show resolved Hide resolved
packages/workflow/src/Interfaces.ts Outdated Show resolved Hide resolved
packages/cli/src/NodeTypes.ts Outdated Show resolved Hide resolved
packages/nodes-base/scripts/generate-metadata.js Outdated Show resolved Hide resolved
packages/cli/src/CredentialTypes.ts Outdated Show resolved Hide resolved
packages/cli/src/CredentialTypes.ts Outdated Show resolved Hide resolved
packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
this.types.nodes = this.types.nodes.concat(types.nodes);
this.types.credentials = this.types.credentials.concat(types.credentials);

// Copy over all icons and set `iconUrl` for the frontend
Copy link
Contributor

Choose a reason for hiding this comment

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

We are copying icons from /nodes-base/src to /nodes-base/dist with Gulp during build, and here we are copying those icons from /nodes-base/dist to the root dir cache. Can we copy to both destinations in the same step, either with Gulp or otherwise?

Copy link
Member Author

@netroy netroy Nov 22, 2022

Choose a reason for hiding this comment

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

the first part happens during the build process for nodes-base, and the second one happens when loading nodes-base and community nodes that might also have icons.
We need the first step in the build, because we don't publish the icons in the src folder, and we need these icons in the published package.

packages/core/src/DirectoryLoader.ts Outdated Show resolved Hide resolved
@netroy
Copy link
Member Author

netroy commented Nov 22, 2022

  1. This breaks the dev flow for node builders in the team and in the community, because watch transpiles changes to dist but does not reflect them in the root dir cache. We have to keep the cache fresh or bypass it for watch.

Done. for now, we'll be using tsc-watch to regenerate the types after tsc finishes successfully. This however does not update the already-loaded nodes in a running server, and nor does it restart the server. But, that's the same behavior on master as well.
I'd like to make the type-generation script incremental, and would also like to automatically unload nodes/credential classes from a running server, so we don't actually need to restart the server. But, that's another big change, and I'd rather not do that in this PR.
We don't need to worry about refreshing the known files for now either as we still need to add files names explicitly in the n8n block in the package.json. I plan to tackle that as well soon.

  • In /dist/known/credentials.json the mapping from credential type name to className and sourcePath looks inferrable. Do we need these two details to load a credential type? In /dist/known/nodes.json, the mapping from node type name to className looks inferrable but sourcePath becomes unpredictable because of grouping dirs like Aws, which btw I am not sure we benefit much from.

In master we are inferring className from the fileName. I changed this to avoid additional string manipulation and garbage generation at startup. But, I'm not sure if the impact of that is significant. If you prefer, I can remove className from the generated files, and infer it before setting it in the objects stores in this.known in /home/aditya/Work/n8n/n8n/packages/cli/src/LoadNodesAndCredentials.ts. WDYT?

@netroy netroy merged commit b6c57e1 into master Nov 23, 2022
@netroy netroy deleted the N8N-4532-lazy-loading branch November 23, 2022 15:20
@n8n-assistant n8n-assistant bot added the Upcoming Release Will be part of the upcoming release label Nov 23, 2022
@janober
Copy link
Member

janober commented Nov 24, 2022

Got released with [email protected]

@janober janober removed the Upcoming Release Will be part of the upcoming release label Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team ui Enhancement in /editor-ui or /design-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants