feat: Only send needed data to task runner (no-changelog) #11487
+1,511
−252
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When executing JS code in the Code Node, the task runner has currently been fetching the entire workflow execution context data. This can be a lot of data, and can cause OOMs on large workflows. Most often Code Node is used in such a way that it only uses the input data and maaaaybe some other node's data. Hence sending all the data is an overkill. This PR changes the behaviour to send only the needed data. This is implemented by running a static analysis of the code and identifying which built-in variables are accessed within the code. Based on that analysis we only send the needed data. There can be corner cases where we can't statically analyse for example which nodes' data is needed. This can happen for example when a variable is used as parameter to $() function. In these cases we send all the data. Based on some naive measurements, the message size is reduced something between 30-70%. Next steps: After this change there is room for optimization. For example, there is still duplication in the message send to the task runner. The input data of the Code Node is located in multiple parts of the message, which all end up being separate objects when deserialized in the task runner.
tomi
changed the title
n8n-assistant
bot
added
core
ivov
reviewed
Nov 1, 2024
Comment on lines
96
to
98
| if (accessedProperty.value === 'item') { | ||
| state.markNeedsAllNodes(); | ||
| } |
There was a problem hiding this comment.
I think also pairedItem and itemMatching need to trace back the chain.
n8n/packages/workflow/src/WorkflowDataProxy.ts
Line 1027 in 643d66c
There was a problem hiding this comment.
Good catch! Will add that
packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/built-ins-parser.ts
Show resolved
Hide resolved
packages/cli/src/runners/task-managers/data-request-response-builder.ts
Outdated
Show resolved
Hide resolved
packages/cli/src/runners/task-managers/data-request-response-builder.ts
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /** | ||
| * Assuming the given `obj` is an object where the keys are node names, |
There was a problem hiding this comment.
Would something break if the string is not a valid node name? e.g. user typo
There was a problem hiding this comment.
Good question. In that case there is no node with that name and filtering works as expected
packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/built-ins-parser.ts
Show resolved
Hide resolved
...n8n/task-runner/src/js-task-runner/built-ins-parser/__tests__/built-ins-parser-state.test.ts
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When executing JS code in the Code Node, the task runner has currently been fetching the entire workflow execution context data. This can be a lot of data, and can cause OOMs on large workflows. Most often Code Node is used in such a way that it only uses the input data and maaaaybe some other node's data. Hence sending all the data is an overkill.
This PR changes the behaviour to send only the needed data. This is implemented by running a static analysis of the code and identifying which built-in variables are accessed within the code. Based on that analysis we only send the needed data.
There can be corner cases where we can't statically analyse for example which nodes' data is needed. This can happen for example when a variable is used as parameter to $() function. In these cases we send all the data.
Based on some naive measurements, the message size is reduced something between 30-70%.
Next steps:
After this change there is room for optimization. For example, there is still duplication in the message send to the task runner. The input data of the Code Node is located in multiple parts of the message, which all end up being separate objects when deserialized in the task runner.
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/PAY-2174/send-only-needed-data-to-the-runner
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)