Refactor ContentSources to RouteTree for more efficient routing #5360
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
|
✅ This change can build |
🟢 CI successful 🟢Thanks |
Linux Benchmark for a20cb2bClick to view benchmark
|
Linux Benchmark for 6870debClick to view benchmark
|
Linux Benchmark for 3fe8321Click to view benchmark
|
MacOS Benchmark for 3fe8321
Click to view full benchmark
|
Linux Benchmark for d7f68e2Click to view benchmark
|
| let last_tree = tree_values.pop().unwrap(); | ||
| 'outer: while common_base < last_tree.base.len() { | ||
| for tree in tree_values.iter() { | ||
| if tree.base.len() <= common_base { |
There was a problem hiding this comment.
Shouldn't the < case be an invariant at this point? Maybe a debug_assert?
There was a problem hiding this comment.
No, we would have to change the outer's loop condition to be something like while common_base < min_base_len, with let min_base_len = tree_values.iter().min_by_key(…).unwrap().base.len()
| } | ||
| common_base += 1; | ||
| } | ||
| tree_values.push(last_tree); |
There was a problem hiding this comment.
Did we need to pop it in the first place?
There was a problem hiding this comment.
Technically not, but without that we would need to skip the last element of an iterator, which has more code.
MacOS Benchmark for d7f68e2
Click to view full benchmark
|
| let last_tree = tree_values.pop().unwrap(); | ||
| 'outer: while common_base < last_tree.base.len() { | ||
| for tree in tree_values.iter() { | ||
| if tree.base.len() <= common_base { |
There was a problem hiding this comment.
No, we would have to change the outer's loop condition to be something like while common_base < min_base_len, with let min_base_len = tree_values.iter().min_by_key(…).unwrap().base.len()
| catch_all_sources: Vec<GetContentSourceContentVc>, | ||
| fallback_sources: Vec<GetContentSourceContentVc>, |
There was a problem hiding this comment.
Nit: Isn't a fallback just a catchall?
There was a problem hiding this comment.
Fallback is currently unused. I just migrated it from Specificity::Fallback.
There was a problem hiding this comment.
What I'm saying is that it's unnecessary with this scheme, every fallback could be a catchall instead, right?
There was a problem hiding this comment.
There is a slight difference in ordering. All catch alls will match before all fallbacks. If you merge them, they will match in order of definition.
| match &rewrite.ty { | ||
| RewriteType::Location { path_and_query } => { | ||
| let new_uri = Uri::try_from(path_and_query)?; | ||
| let new_asset_path = | ||
| urlencoding::decode(&new_uri.path()[1..])?.into_owned(); | ||
| request_overwrites.uri = new_uri; | ||
| current_asset_path = new_asset_path; | ||
| continue 'routes; | ||
| } | ||
| RewriteType::ContentSource { | ||
| source, | ||
| path_and_query, | ||
| } => { |
There was a problem hiding this comment.
I this this rewrite logic creates a lot of inconsistencies. Before we were rewriting the current sources's URL only, but now we're changing the URL for the current route and all routes that follow. I think it has to only rewrite the the URL for the current route tree, right?
There was a problem hiding this comment.
If referring to the next route in sources, on Rewrite we do no longer apply following routes. The continue 'routes; will skip all remaining routes and starts applying a new route tree with the new URL.
The Rewrite is basically counted as route match.
| RewriteType::Sources { | ||
| sources: new_sources, | ||
| } => { | ||
| sources = *new_sources; | ||
| continue 'sources; | ||
| } |
There was a problem hiding this comment.
And this creates similar inconsistencies. There could be more sources after the current source that would give a valid result, but we'll never reach those. I think we need to only sources = new_sources.concat(remaining_sources) here
There was a problem hiding this comment.
Same, the Rewrite response is counted as route match and no following source is applied.
e. g. for /some/path and /some/[dynamic] returned by get_routes, once /some/path returns a Rewrite, /some/[dynamic] will no longer be applied
Linux Benchmark for 4fb7bd1Click to view benchmark
|
Windows Benchmark for 4fb7bd1
Click to view full benchmark
|
Linux Benchmark for d23d03dClick to view benchmark
|
MacOS Benchmark for d23d03d
Click to view full benchmark
|
Linux Benchmark for 4ecb6d2Click to view benchmark
|
MacOS Benchmark for 4ecb6d2
Click to view full benchmark
|
| @@ -91,6 +93,14 @@ pub enum ContentSourceContent { | |||
| Rewrite(RewriteVc), | |||
| } | |||
|
|
|||
| /// This trait can be emitted as collectible and will be applied after the | |||
| /// request is handled and it's ensured that it finishes before the next request | |||
| /// is handled. | |||
There was a problem hiding this comment.
Can you explain why you would want to do emit side effect vcs? What does it accomplish?
There was a problem hiding this comment.
It moves the invalidation to a time after the request is sent to the user. This delays all computation for the state change to some time after this request and before the next request.
If we would just change it inline, the strongly consistent read from the request handling would wait for this invalidation to be completed as it might have effects on the current request (but it doesn't have these).
So for expensive invalidations this allows to respond ealier.
There was a problem hiding this comment.
Technically it's also possible to return a list of SideEffectVcs, but this requires adding a list to all intermediate functions, which is quite a large change. Since we have to cool system of collectibles we can use that. It allows to emit a side effect from anywhere and the server will make sure that it's handled at the right time.
| /// A unresolve asset. We need to have a unresolve Asset here as we need to | ||
| /// lookup the Vc identity in the expanded set. |
There was a problem hiding this comment.
Sorry, I still don't understand. What is it about a resolved asset that prevents vc identity lookup? How does an unresolved asset solve that?
There was a problem hiding this comment.
This should hopefully explain it: 7bfe700
| catch_all_sources: Vec<GetContentSourceContentVc>, | ||
| fallback_sources: Vec<GetContentSourceContentVc>, |
There was a problem hiding this comment.
What I'm saying is that it's unnecessary with this scheme, every fallback could be a catchall instead, right?
| let sources = sources | ||
| .await? | ||
| .iter() | ||
| .map(|s| WrappedGetContentSourceContentVc::new(*s, processor).into()) | ||
| .collect(); |
There was a problem hiding this comment.
Leave a TODO with linear task, please.
Linux Benchmark for 0f81f6aClick to view benchmark
|
MacOS Benchmark for 0f81f6a
Click to view full benchmark
|
Linux Benchmark for fd8784cClick to view benchmark
|
MacOS Benchmark for fd8784c
Click to view full benchmark
|
### What? This fixes a performance problem when many pages are involved. fixes WEB-1067 see also vercel/turborepo#5360 ### Turbopack Changes * vercel/turborepo#5416 * vercel/turborepo#5360
…el/turborepo#5360) ### Description Routing is inefficient with many pages next.js PR: #51660
…el/turborepo#5360) ### Description Routing is inefficient with many pages next.js PR: #51660
…el/turborepo#5360) ### Description Routing is inefficient with many pages next.js PR: #51660
Description
Routing is inefficient with many pages
next.js PR: vercel/next.js#51660