-
Notifications
You must be signed in to change notification settings - Fork 27.4k
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
refactor(turbo-tasks): Convert local Vcs to non-local Vcs when returning from task functions #74714
base: bgw/vcarc-uses-operationvc
Are you sure you want to change the base?
refactor(turbo-tasks): Convert local Vcs to non-local Vcs when returning from task functions #74714
Conversation
fn $get_args_fn<$($arg: TaskInput + 'static,)*>(arg: &dyn MagicAny) -> Result<($($arg,)*)> { | ||
match arg.downcast_ref::<($($arg,)*)>() { | ||
Some(($($arg,)*)) => Ok(($($arg.clone(),)*)), | ||
None => { | ||
#[cfg(debug_assertions)] | ||
let context_str = get_debug_downcast_error_msg::<($($arg,)*)>(arg); | ||
#[cfg(not(debug_assertions))] | ||
let context_str = "Invalid argument type"; | ||
|
||
anyhow::bail!(context_str); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved this inside the macro, which lets us deduplicate the .clone()
calls, instead of putting them in each individual trait implementation.
impl<F, Output, Recv, $($arg,)*> TaskFnInputFunctionWithThis<OperationMode, Recv, ($($arg,)*)> for F | ||
where | ||
Recv: Sync + Send + 'static, | ||
$($arg: TaskInput + 'static,)* | ||
F: Fn(OperationVc<Recv>, $($arg,)*) -> Output + Send + Sync + Clone + 'static, | ||
Output: TaskOutput + 'static, | ||
{ | ||
#[allow(non_snake_case)] | ||
fn functor(&self, this: RawVc, arg: &dyn MagicAny) -> Result<NativeTaskFuture> { | ||
let task_fn = self.clone(); | ||
let recv = OperationVc::<Recv>::from(this); | ||
|
||
let ($($arg,)*) = get_args::<($($arg,)*)>(arg)?; | ||
$( | ||
let $arg = $arg.clone(); | ||
)* | ||
|
||
let ($($arg,)*) = $get_args_fn::<$($arg,)*>(arg)?; | ||
Ok(Box::pin(async move { | ||
Output::try_into_raw_vc((task_fn)(recv, $($arg,)*)) | ||
let output = (task_fn)(recv, $($arg,)*); | ||
output_try_into_non_local_raw_vc(output).await | ||
})) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up not implementing support for methods with #[turbo_tasks::function(operation)]
, given that there's a limited number of callsites, and dynamic dispatch likely couldn't work with methods taking self: OperationVc
anyways.
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
buildDuration | 30.7s | 25.2s | N/A |
buildDurationCached | 27.5s | 24.4s | N/A |
nodeModulesSize | 417 MB | 417 MB | ✓ |
nextStartRea..uration (ms) | 850ms | 717ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
5306-HASH.js gzip | 53.4 kB | 53.4 kB | N/A |
8276.HASH.js gzip | 169 B | 168 B | N/A |
8377-HASH.js gzip | 5.44 kB | 5.44 kB | N/A |
bccd1874-HASH.js gzip | 52.9 kB | 52.9 kB | N/A |
framework-HASH.js gzip | 57.5 kB | 57.5 kB | N/A |
main-app-HASH.js gzip | 240 B | 242 B | N/A |
main-HASH.js gzip | 34.2 kB | 34.2 kB | N/A |
webpack-HASH.js gzip | 1.71 kB | 1.71 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 39.4 kB | 39.4 kB | ✓ |
Overall change | 39.4 kB | 39.4 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
_app-HASH.js gzip | 193 B | 193 B | ✓ |
_error-HASH.js gzip | 193 B | 193 B | ✓ |
amp-HASH.js gzip | 512 B | 510 B | N/A |
css-HASH.js gzip | 343 B | 342 B | N/A |
dynamic-HASH.js gzip | 1.84 kB | 1.84 kB | ✓ |
edge-ssr-HASH.js gzip | 265 B | 265 B | ✓ |
head-HASH.js gzip | 363 B | 362 B | N/A |
hooks-HASH.js gzip | 393 B | 392 B | N/A |
image-HASH.js gzip | 4.57 kB | 4.57 kB | N/A |
index-HASH.js gzip | 268 B | 268 B | ✓ |
link-HASH.js gzip | 2.35 kB | 2.34 kB | N/A |
routerDirect..HASH.js gzip | 328 B | 328 B | ✓ |
script-HASH.js gzip | 397 B | 397 B | ✓ |
withRouter-HASH.js gzip | 323 B | 326 B | N/A |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 3.59 kB | 3.59 kB | ✓ |
Client Build Manifests
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
_buildManifest.js gzip | 749 B | 747 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
index.html gzip | 524 B | 523 B | N/A |
link.html gzip | 539 B | 538 B | N/A |
withRouter.html gzip | 520 B | 520 B | ✓ |
Overall change | 520 B | 520 B | ✓ |
Edge SSR bundle Size
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
edge-ssr.js gzip | 129 kB | 129 kB | N/A |
page.js gzip | 207 kB | 207 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 669 B | 669 B | ✓ |
middleware-r..fest.js gzip | 155 B | 156 B | N/A |
middleware.js gzip | 31.3 kB | 31.3 kB | N/A |
edge-runtime..pack.js gzip | 844 B | 844 B | ✓ |
Overall change | 1.51 kB | 1.51 kB | ✓ |
Next Runtimes
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
274-experime...dev.js gzip | 322 B | 322 B | ✓ |
274.runtime.dev.js gzip | 314 B | 314 B | ✓ |
app-page-exp...dev.js gzip | 369 kB | 369 kB | ✓ |
app-page-exp..prod.js gzip | 130 kB | 130 kB | ✓ |
app-page-tur..prod.js gzip | 142 kB | 142 kB | ✓ |
app-page-tur..prod.js gzip | 138 kB | 138 kB | ✓ |
app-page.run...dev.js gzip | 357 kB | 357 kB | ✓ |
app-page.run..prod.js gzip | 126 kB | 126 kB | ✓ |
app-route-ex...dev.js gzip | 37.6 kB | 37.6 kB | ✓ |
app-route-ex..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.6 kB | 25.6 kB | ✓ |
app-route-tu..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
app-route.ru...dev.js gzip | 39.2 kB | 39.2 kB | ✓ |
app-route.ru..prod.js gzip | 25.4 kB | 25.4 kB | ✓ |
pages-api-tu..prod.js gzip | 9.69 kB | 9.69 kB | ✓ |
pages-api.ru...dev.js gzip | 11.6 kB | 11.6 kB | ✓ |
pages-api.ru..prod.js gzip | 9.68 kB | 9.68 kB | ✓ |
pages-turbo...prod.js gzip | 21.7 kB | 21.7 kB | ✓ |
pages.runtim...dev.js gzip | 27.5 kB | 27.5 kB | ✓ |
pages.runtim..prod.js gzip | 21.7 kB | 21.7 kB | ✓ |
server.runti..prod.js gzip | 916 kB | 916 kB | ✓ |
Overall change | 2.46 MB | 2.46 MB | ✓ |
build cache Overall increase ⚠️
vercel/next.js canary | vercel/next.js bgw/return-values-should-be-non-local | Change | |
---|---|---|---|
0.pack gzip | 2.09 MB | 2.09 MB | |
index.pack gzip | 74.8 kB | 75.1 kB | |
Overall change | 2.17 MB | 2.17 MB |
Diff details
Diff for main-HASH.js
Diff too large to display
fn get_args<T: MagicAny>(arg: &dyn MagicAny) -> Result<&T> { | ||
let value = arg.downcast_ref::<T>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you could have archived the same with:
-fn get_args<T: MagicAny>(arg: &dyn MagicAny) -> Result<&T> {
+fn get_args<T: MagicAny + Clone>(arg: &dyn MagicAny) -> Result<T> {
- let value = arg.downcast_ref::<T>();
+ let value = arg.downcast_ref::<T>().clone();
Since the tuple could be cloned
But not sure...
d2c1eab
to
56c77c9
Compare
110001f
to
1f21777
Compare
Failing test suitesCommit: 40117d9
Expand output● next.rs api › should allow to write root page to disk
● next.rs api › should allow to write root page to disk
● next.rs api › should allow to write pages edge api to disk
● next.rs api › should allow to write pages Node.js api to disk
● next.rs api › should allow to write app edge page to disk
● next.rs api › should allow to write app edge page to disk
● next.rs api › should allow to write app Node.js page to disk
● next.rs api › should allow to write app Node.js page to disk
● next.rs api › should allow to write pages edge page to disk
● next.rs api › should allow to write pages edge page to disk
● next.rs api › should allow to write pages Node.js page to disk
● next.rs api › should allow to write pages Node.js page to disk
● next.rs api › should allow to write app edge route to disk
● next.rs api › should allow to write app Node.js route to disk
Read more about building and testing Next.js in contributing.md.
Expand output● persistent-caching › should persistent cache loaders
Read more about building and testing Next.js in contributing.md. |
56c77c9
to
3ca36f6
Compare
1f21777
to
1a10536
Compare
3ca36f6
to
d930890
Compare
1a10536
to
59ff3b4
Compare
d930890
to
fc3f3d2
Compare
59ff3b4
to
1713534
Compare
fc3f3d2
to
968aa54
Compare
1713534
to
90c2d5c
Compare
968aa54
to
9585d5e
Compare
90c2d5c
to
fe63d1b
Compare
9585d5e
to
2d433d3
Compare
fe63d1b
to
0e91d34
Compare
2d433d3
to
ff5cce9
Compare
0e91d34
to
fb08b7c
Compare
ff5cce9
to
03c98a4
Compare
fb08b7c
to
c7279e4
Compare
03c98a4
to
f8efda7
Compare
c7279e4
to
b2e9360
Compare
f8efda7
to
9c94863
Compare
b2e9360
to
9c6d5d4
Compare
9c94863
to
9b82562
Compare
9c6d5d4
to
78f1579
Compare
9b82562
to
006a836
Compare
78f1579
to
073dfc1
Compare
006a836
to
d580123
Compare
073dfc1
to
6ed1846
Compare
8ca6dfa
to
f6ab4a3
Compare
…ing from task functions
6ed1846
to
40117d9
Compare
No description provided.