Refactor GraphTraversal to avoid non-determinism #4098
Merged
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
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
9 Ignored Deployments
|
alexkirsz
commented
Mar 7, 2023
| self.chunk_items_count += 1; | ||
| } | ||
| map_children.push(child); | ||
| } |
There was a problem hiding this comment.
The "else" branch here was the issue: ChunkContentVisit::map_children could return different children depending on whether an asset was already processed or not.
🟢 CI successful 🟢Thanks |
Benchmark for 348ee46
Click to view full benchmark
|
sokra
approved these changes
Mar 7, 2023
alexkirsz
deleted the
alexkirsz/web-690-chunk_content_internal_parallel-graph
branch
mehulkar
pushed a commit
that referenced
this pull request
Mar 7, 2023
### Description The current design of the `Visit` trait makes it easy to introduce non-determinism by having `map_children` return different results depending on the order in which it's called. For instance, if `map_children` tries to de-duplicate children (as is the case in `ChunkContentVisit`): ``` Order 1: 1. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA, NodeB]) 2. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeC]) Order 2: 1. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeB, NodeC]) 2. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA]) ``` These two calling orders will result in different generated graphs, as the first order will forget about the edge Node2 -> NodeB, while the second will forget about the edge Node1 -> NodeB. Instead, this PR makes it so `map_children` is called *after* inserting all nodes into the graph. This ensures that a different ordering can't affect the final shape of the graph. It also refactors the `GraphTraversal::visit` method and the `Visit` trait to make it more consistent with graph terminology and (hopefully) easier to understand. ### Testing Instructions This solves an issue on the branch sokra/repro-for-alex.
kodiakhq bot
pushed a commit
to vercel/next.js
that referenced
this pull request
Mar 8, 2023
## Bugfixes - vercel/turborepo#3526 - vercel/turborepo#4084 - vercel/turborepo#4083 - vercel/turborepo#4067 - vercel/turborepo#3959 - vercel/turborepo#4060 - vercel/turborepo#4081 - vercel/turborepo#4103 - vercel/turborepo#4100 - vercel/turborepo#4108 - vercel/turborepo#4101 - vercel/turborepo#4112 - vercel/turborepo#3956 - vercel/turborepo#4118 - vercel/turborepo#4117 ## Feature - vercel/turborepo#4058 - vercel/turborepo#4013 - vercel/turborepo#4005 - vercel/turborepo#4001 - vercel/turborepo#3839 - vercel/turborepo#4017 - vercel/turborepo#4086 - vercel/turborepo#4067 ## Performance - vercel/turborepo#4040 - vercel/turborepo#4110 ## Refactor - vercel/turborepo#4098
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Jul 25, 2024
### Description The current design of the `Visit` trait makes it easy to introduce non-determinism by having `map_children` return different results depending on the order in which it's called. For instance, if `map_children` tries to de-duplicate children (as is the case in `ChunkContentVisit`): ``` Order 1: 1. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA, NodeB]) 2. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeC]) Order 2: 1. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeB, NodeC]) 2. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA]) ``` These two calling orders will result in different generated graphs, as the first order will forget about the edge Node2 -> NodeB, while the second will forget about the edge Node1 -> NodeB. Instead, this PR makes it so `map_children` is called *after* inserting all nodes into the graph. This ensures that a different ordering can't affect the final shape of the graph. It also refactors the `GraphTraversal::visit` method and the `Visit` trait to make it more consistent with graph terminology and (hopefully) easier to understand. ### Testing Instructions This solves an issue on the branch sokra/repro-for-alex.
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Jul 29, 2024
### Description The current design of the `Visit` trait makes it easy to introduce non-determinism by having `map_children` return different results depending on the order in which it's called. For instance, if `map_children` tries to de-duplicate children (as is the case in `ChunkContentVisit`): ``` Order 1: 1. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA, NodeB]) 2. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeC]) Order 2: 1. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeB, NodeC]) 2. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA]) ``` These two calling orders will result in different generated graphs, as the first order will forget about the edge Node2 -> NodeB, while the second will forget about the edge Node1 -> NodeB. Instead, this PR makes it so `map_children` is called *after* inserting all nodes into the graph. This ensures that a different ordering can't affect the final shape of the graph. It also refactors the `GraphTraversal::visit` method and the `Visit` trait to make it more consistent with graph terminology and (hopefully) easier to understand. ### Testing Instructions This solves an issue on the branch sokra/repro-for-alex.
ForsakenHarmony
pushed a commit
to vercel/next.js
that referenced
this pull request
Aug 1, 2024
### Description The current design of the `Visit` trait makes it easy to introduce non-determinism by having `map_children` return different results depending on the order in which it's called. For instance, if `map_children` tries to de-duplicate children (as is the case in `ChunkContentVisit`): ``` Order 1: 1. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA, NodeB]) 2. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeC]) Order 2: 1. (from parent Node2) map_children([NodeB, NodeC]) -> GraphTraversalControlFlow::Continue([NodeB, NodeC]) 2. (from parent Node1) map_children([NodeA, NodeB]) -> GraphTraversalControlFlow::Continue([NodeA]) ``` These two calling orders will result in different generated graphs, as the first order will forget about the edge Node2 -> NodeB, while the second will forget about the edge Node1 -> NodeB. Instead, this PR makes it so `map_children` is called *after* inserting all nodes into the graph. This ensures that a different ordering can't affect the final shape of the graph. It also refactors the `GraphTraversal::visit` method and the `Visit` trait to make it more consistent with graph terminology and (hopefully) easier to understand. ### Testing Instructions This solves an issue on the branch sokra/repro-for-alex.
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.
Description
The current design of the
Visittrait makes it easy to introduce non-determinism by havingmap_childrenreturn different results depending on the order in which it's called.For instance, if
map_childrentries to de-duplicate children (as is the case inChunkContentVisit):These two calling orders will result in different generated graphs, as the first order will forget about the edge Node2 -> NodeB, while the second will forget about the edge Node1 -> NodeB.
Instead, this PR makes it so
map_childrenis called after inserting all nodes into the graph. This ensures that a different ordering can't affect the final shape of the graph.It also refactors the
GraphTraversal::visitmethod and theVisittrait to make it more consistent with graph terminology and (hopefully) easier to understand.Testing Instructions
This solves an issue on the branch sokra/repro-for-alex.