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.

Sign up for GitHub

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

Jump to bottom

turbo-tasks this calls #8735

Merged
merged 4 commits into from
Jul 17, 2024
Merged

turbo-tasks this calls #8735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7cb7bf7
make PersistentTaskType::Native a named struct
sokra Jul 8, 2024
e059ad0
make PersistentTaskType::ResolveNative a named struct
sokra Jul 8, 2024
16f4044
make PersistentTaskType::ResolveTrait a named struct
sokra Jul 8, 2024
58d79f4
make this calls explicit and add this argument
sokra Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 97 additions & 34 deletions crates/turbo-tasks-macros/src/func.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ pub struct TurboFn {
// block: Block,
ident: Ident,
output: Type,
this: Option<Input>,
inputs: Vec<Input>,
}

Expand Down Expand Up @@ -56,6 +57,7 @@ impl TurboFn {
}

let mut raw_inputs = original_signature.inputs.iter();
let mut this = None;
let mut inputs = Vec::with_capacity(raw_inputs.len());

if let Some(possibly_receiver) = raw_inputs.next() {
Expand Down Expand Up @@ -142,7 +144,7 @@ impl TurboFn {
_ => {}
}

inputs.push(Input {
this = Some(Input {
ident: Ident::new("self", self_token.span()),
ty: parse_quote! { turbo_tasks::Vc<Self> },
});
Expand All @@ -160,7 +162,7 @@ impl TurboFn {
return None;
}

let ident = if let Pat::Ident(ident) = &*typed.pat {
if let Pat::Ident(ident) = &*typed.pat {
if ident.ident == "self" {
if let DefinitionContext::NakedFn { .. } = definition_context {
// The function is not associated. The compiler will emit an error
Expand All @@ -174,6 +176,13 @@ impl TurboFn {
// if the user provided an invalid receiver type
// when
// calling `into_concrete`.

let ident = ident.ident.clone();

this = Some(Input {
ident,
ty: parse_quote! { turbo_tasks::Vc<Self> },
});
} else {
match definition_context {
DefinitionContext::NakedFn { .. }
Expand All @@ -192,18 +201,22 @@ impl TurboFn {
return None;
}
}
}
let ident = ident.ident.clone();

ident.ident.clone()
inputs.push(Input {
ident,
ty: (*typed.ty).clone(),
});
}
} else {
// We can't support destructuring patterns (or other kinds of patterns).
Ident::new("arg1", typed.pat.span())
};
let ident = Ident::new("arg1", typed.pat.span());

inputs.push(Input {
ident,
ty: (*typed.ty).clone(),
});
inputs.push(Input {
ident,
ty: (*typed.ty).clone(),
});
}
}
}
}
Expand Down Expand Up @@ -234,6 +247,7 @@ impl TurboFn {
Some(TurboFn {
ident: original_signature.ident.clone(),
output,
this,
inputs,
})
}
Expand All @@ -242,8 +256,10 @@ impl TurboFn {
/// converted to a standard turbo_tasks function signature.
pub fn signature(&self) -> Signature {
let exposed_inputs: Punctuated<_, Token![,]> = self
.inputs
.iter()
.this
.as_ref()
.into_iter()
.chain(self.inputs.iter())
.map(|input| {
FnArg::Typed(PatType {
attrs: Default::default(),
Expand Down Expand Up @@ -288,21 +304,38 @@ impl TurboFn {
.collect()
}

fn converted_this(&self) -> Option<Expr> {
self.this.as_ref().map(|Input { ty: _, ident }| {
parse_quote! {
turbo_tasks::Vc::into_raw(#ident)
}
})
}

/// The block of the exposed function for a dynamic dispatch call to the
/// given trait.
pub fn dynamic_block(&self, trait_type_id_ident: &Ident) -> Block {
let ident = &self.ident;
let output = &self.output;
let converted_inputs = self.converted_inputs();
parse_quote! {
{
<#output as turbo_tasks::task::TaskOutput>::try_from_raw_vc(
turbo_tasks::trait_call(
*#trait_type_id_ident,
std::borrow::Cow::Borrowed(stringify!(#ident)),
vec![#converted_inputs],
if let Some(converted_this) = self.converted_this() {
let converted_inputs = self.converted_inputs();
parse_quote! {
{
<#output as turbo_tasks::task::TaskOutput>::try_from_raw_vc(
turbo_tasks::trait_call(
*#trait_type_id_ident,
std::borrow::Cow::Borrowed(stringify!(#ident)),
#converted_this,
vec![#converted_inputs],
)
)
)
}
}
} else {
parse_quote! {
{
unimplemented!("trait methods without self are not yet supported")
}
}
}
Comment on lines +334 to 340
Copy link
Member

Choose a reason for hiding this comment

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

  • This might be cleaner as a let else statement
  • What do we mean by "not yet"? Trait methods without self don't make sense. That's not a method, it's just an associated function (and dynamic dispatch isn't applicable)
  • Maybe this should be a compile_error!()? https://doc.rust-lang.org/std/macro.compile_error.html

}
Expand All @@ -312,17 +345,35 @@ impl TurboFn {
pub fn static_block(&self, native_function_id_ident: &Ident) -> Block {
let output = &self.output;
let converted_inputs = self.converted_inputs();
parse_quote! {
{
<#output as turbo_tasks::task::TaskOutput>::try_from_raw_vc(
turbo_tasks::dynamic_call(
*#native_function_id_ident,
vec![#converted_inputs],
if let Some(converted_this) = self.converted_this() {
parse_quote! {
{
<#output as turbo_tasks::task::TaskOutput>::try_from_raw_vc(
turbo_tasks::dynamic_this_call(
*#native_function_id_ident,
#converted_this,
vec![#converted_inputs],
)
)
)
}
}
} else {
parse_quote! {
{
<#output as turbo_tasks::task::TaskOutput>::try_from_raw_vc(
turbo_tasks::dynamic_call(
*#native_function_id_ident,
vec![#converted_inputs],
)
)
}
}
}
}

pub(crate) fn is_method(&self) -> bool {
self.this.is_some()
}
}

fn return_type_to_type(return_type: &ReturnType) -> Type {
Expand Down Expand Up @@ -454,13 +505,15 @@ impl DefinitionContext {
pub struct NativeFn {
function_path_string: String,
function_path: ExprPath,
is_method: bool,
}

impl NativeFn {
pub fn new(function_path_string: &str, function_path: &ExprPath) -> NativeFn {
pub fn new(function_path_string: &str, function_path: &ExprPath, is_method: bool) -> NativeFn {
NativeFn {
function_path_string: function_path_string.to_owned(),
function_path: function_path.clone(),
is_method,
}
}

Expand All @@ -472,13 +525,23 @@ impl NativeFn {
let Self {
function_path_string,
function_path,
is_method,
} = self;

parse_quote! {
turbo_tasks::macro_helpers::Lazy::new(|| {
#[allow(deprecated)]
turbo_tasks::NativeFunction::new(#function_path_string.to_owned(), #function_path)
})
if *is_method {
parse_quote! {
turbo_tasks::macro_helpers::Lazy::new(|| {
#[allow(deprecated)]
turbo_tasks::NativeFunction::new_method(#function_path_string.to_owned(), #function_path)
})
}
} else {
parse_quote! {
turbo_tasks::macro_helpers::Lazy::new(|| {
#[allow(deprecated)]
turbo_tasks::NativeFunction::new_function(#function_path_string.to_owned(), #function_path)
})
}
}
}

Expand Down
6 changes: 5 additions & 1 deletion crates/turbo-tasks-macros/src/function_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,11 @@ pub fn function(_args: TokenStream, input: TokenStream) -> TokenStream {
let mut inline_signature = sig.clone();
inline_signature.ident = inline_function_ident;

let native_fn = NativeFn::new(&ident.to_string(), &inline_function_path);
let native_fn = NativeFn::new(
&ident.to_string(),
&inline_function_path,
turbo_fn.is_method(),
);
let native_function_ident = get_native_function_ident(ident);
let native_function_ty = native_fn.ty();
let native_function_def = native_fn.definition();
Expand Down
7 changes: 7 additions & 0 deletions crates/turbo-tasks-macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,13 @@ pub fn function(args: TokenStream, input: TokenStream) -> TokenStream {
function_macro::function(args, input)
}

#[allow_internal_unstable(min_specialization, into_future, trivial_bounds)]
#[proc_macro_error]
#[proc_macro_attribute]
pub fn test_tt(_args: TokenStream, input: TokenStream) -> TokenStream {
derive::derive_value_debug(input)
}

Comment on lines +169 to +175
Copy link
Member

Choose a reason for hiding this comment

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

Is this left over from your testing? This doesn't look like it's called anywhere in the stack of PRs?

#[allow_internal_unstable(min_specialization, into_future, trivial_bounds)]
#[proc_macro_error]
#[proc_macro_attribute]
Expand Down
2 changes: 2 additions & 0 deletions crates/turbo-tasks-macros/src/value_impl_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream {
let native_fn = NativeFn::new(
&format!("{ty}::{ident}", ty = ty.to_token_stream()),
&inline_function_path,
turbo_fn.is_method(),
);

let native_function_ident = get_inherent_impl_function_ident(ty_ident, ident);
Expand Down Expand Up @@ -225,6 +226,7 @@ pub fn value_impl(args: TokenStream, input: TokenStream) -> TokenStream {
trait_path = trait_path.to_token_stream()
),
&inline_function_path,
turbo_fn.is_method(),
);

let native_function_ident =
Expand Down
7 changes: 5 additions & 2 deletions crates/turbo-tasks-macros/src/value_trait_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,11 @@ pub fn value_trait(args: TokenStream, input: TokenStream) -> TokenStream {
let mut inline_signature = sig.clone();
inline_signature.ident = inline_function_ident;

let native_function =
NativeFn::new(&format!("{trait_ident}::{ident}"), &inline_function_path);
let native_function = NativeFn::new(
&format!("{trait_ident}::{ident}"),
&inline_function_path,
turbo_fn.is_method(),
);

let native_function_ident = get_trait_default_impl_function_ident(trait_ident, ident);
let native_function_ty = native_function.ty();
Expand Down
35 changes: 25 additions & 10 deletions crates/turbo-tasks-memory/src/memory_backend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,12 +545,25 @@ impl Backend for MemoryBackend {
{
// fast pass without creating a new task
self.task_statistics().map(|stats| match &*task_type {
PersistentTaskType::ResolveNative(function_id, ..)
| PersistentTaskType::Native(function_id, ..) => {
PersistentTaskType::ResolveNative {
fn_type: function_id,
this: _,
args: _,
}
| PersistentTaskType::Native {
fn_type: function_id,
this: _,
args: _,
} => {
stats.increment_cache_hit(*function_id);
}
PersistentTaskType::ResolveTrait(trait_type, name, inputs) => {
// HACK: Resolve the first argument (`self`) in order to attribute the cache hit
PersistentTaskType::ResolveTrait {
trait_type,
method_name: name,
this,
args: _,
} => {
// HACK: Resolve the this argument (`self`) in order to attribute the cache hit
// to the concrete trait implementation, rather than the dynamic trait method.
// This ensures cache hits and misses are both attributed to the same thing.
//
Expand All @@ -565,10 +578,7 @@ impl Backend for MemoryBackend {
// ResolveTrait tasks.
let trait_type = *trait_type;
let name = name.clone();
let this = inputs
.first()
.cloned()
.expect("No arguments for trait call");
let this = *this;
let stats = Arc::clone(stats);
turbo_tasks.run_once(Box::pin(async move {
let function_id =
Expand All @@ -582,10 +592,15 @@ impl Backend for MemoryBackend {
task
} else {
self.task_statistics().map(|stats| match &*task_type {
PersistentTaskType::Native(function_id, ..) => {
PersistentTaskType::Native {
fn_type: function_id,
this: _,
args: _,
} => {
stats.increment_cache_miss(*function_id);
}
PersistentTaskType::ResolveTrait(..) | PersistentTaskType::ResolveNative(..) => {
PersistentTaskType::ResolveTrait { .. }
| PersistentTaskType::ResolveNative { .. } => {
// these types re-execute themselves as `Native` after
// resolving their arguments, skip counting their
// executions here to avoid double-counting
Expand Down
Loading
Loading