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.

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

Refined .call() and .construct() API #817

Closed
wants to merge 8 commits into from
Closed

Conversation

dherman
Copy link
Collaborator

@dherman dherman commented Oct 29, 2021

This PR experiments with a refined .call() and .construct() API for JsFunction, based on the ideas we came up with in #796 as well as the idea of overloading result types to make downcasting more concise and ergonomic. The changes include:

  • Uses the Arguments trait instead of IntoIterator, which still works for Vec and arrays but also heterogeneous tuples
  • Overloads and downcasts to the result type
  • Adds the .exec() method for discarding the result when it's not needed

This generally leads to more concise and readable code. But it has a few downsides:

  • The difference between arrays and tuples is subtle: heterogeneous vs homogeneous types, and the fact that empty arrays ([]) tend to lead to inference errors where empty tuples (()) usually work, could both be confusing especially to newer Rust programmers.
  • You still tend to end up having to compute arguments in previous statements in order to use cx, whereas the method chaining approach in Arguments builder API #796 circumvents this issue.
  • Downcasting with turbofish requires three blank type parameters (_) to be passed after the result type.
  • Always downcasting, even when the result type is JsValue, may not be able to be made zero cost. (But this is probably most often needed for discarding the result, in which case .exec() avoids the downcast overhead.)

I wanted to implement this before we finalize the builder APIs, so that we can consider the complete set of function call APIs holistically.

@dherman dherman requested a review from kjvalencik October 29, 2021 17:57
@dherman dherman mentioned this pull request Oct 29, 2021
f.call(&mut cx, null, args)?
.downcast::<JsNumber, _>(&mut cx)
.or_throw(&mut cx)
f.call::<JsNumber, _, _, _>(&mut cx, null, [arg])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f.call::<JsNumber, _, _, _>(&mut cx, null, [arg])
f.call(&mut cx, null, [arg])

.call(&mut cx, o.upcast::<JsValue>(), args)?
.downcast::<JsNumber, _>(&mut cx)
.or_throw(&mut cx)
get_utc_full_year_method.call::<JsNumber, _, _, _>(&mut cx, o, ())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
get_utc_full_year_method.call::<JsNumber, _, _, _>(&mut cx, o, ())
get_utc_full_year_method.call(&mut cx, o, ())

f.call(&mut cx, null, args)?
.downcast::<JsNumber>()
.or_throw(&mut cx)
f.call::<JsNumber, _, _, _>(&mut cx, null, [arg])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f.call::<JsNumber, _, _, _>(&mut cx, null, [arg])
f.call(&mut cx, null, [arg])

.call(&mut cx, o.upcast::<JsValue>(), args)?
.downcast::<JsNumber>()
.or_throw(&mut cx)
get_utc_full_year_method.call::<JsNumber, _, _, _>(&mut cx, o, ())
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
get_utc_full_year_method.call::<JsNumber, _, _, _>(&mut cx, o, ())
get_utc_full_year_method.call(&mut cx, o, ())

Comment on lines +19 to +23
let mut args = smallvec![];
for arg in self {
args.push(arg.upcast());
}
args
Copy link
Member

Choose a reason for hiding this comment

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

Rust is sometimes able to optimize iterators a little better. SmallVec implements from_iter, so it can be used with collect.

Suggested change
let mut args = smallvec![];
for arg in self {
args.push(arg.upcast());
}
args
self.into_iter().map(|v| v.upcast()).collect()

Comment on lines +31 to +35
let mut args = smallvec![];
for arg in self {
args.push(arg.upcast());
}
args
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut args = smallvec![];
for arg in self {
args.push(arg.upcast());
}
args
self.iter().map(|v| v.upcast()).collect()


impl<'a, T: Value> Arguments<'a> for Vec<Handle<'a, T>> {}

impl<'a, T: Value, const N: usize> private::ArgumentsInternal<'a> for [Handle<'a, T>; N] {
Copy link
Member

Choose a reason for hiding this comment

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

const generics will require a fairly recent version of Rust (1.51, released in March). Given that we're copying anyway, maybe this only needs to be implemented over slices?

I think this and Vec could be deleted and only leave an implementation for &[Handle<'a, T>].

@kjvalencik
Copy link
Member

@dherman One thing that doesn't feel super great about this is that we're always copying all of the elements, even though for slices, we could be using it directly without allocations.

$(#[$attr1])?
impl<'a, $($tprefix: Value, )* $tname1: Value> private::ArgumentsInternal<'a> for ($(Handle<'a, $tprefix>, )* Handle<'a, $tname1>, ) {
fn into_args_vec(self) -> private::ArgsVec<'a> {
let mut args = smallvec![];
Copy link
Member

Choose a reason for hiding this comment

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

I think this would result in better performance if the items were expanded in the smallvec macro instead of pushing one by one.

@dherman
Copy link
Collaborator Author

dherman commented Oct 29, 2021

I chatted with @kjvalencik and it seems like call_with() and call() are so close in functionality that it's not really worth having both. It seems more valuable to have a lower-level primitive that avoids copies and unnecessary coercions, but it's not clear that it's worth using up the name call() for it.

So a conservative plan for now is:

  • Implement call_with() and construct_with() as the high-level ergonomic API.
  • Deprecate call() and construct() <== meh, even this is premature
  • Write a migration guide and get early user feedback to make sure the migration is feasible

We can explore a lower-level, zero-copy primitive that takes a reference to an array of values and passes that directly to N-API, and we can decide later if we want to call that .call() or something that suggests that it's lower level.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants