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

Add get_or_undefined method to arguments of builtin functions #1490

Closed
jedel1043 opened this issue Aug 21, 2021 · 2 comments · Fixed by #1492
Closed

Add get_or_undefined method to arguments of builtin functions #1490

jedel1043 opened this issue Aug 21, 2021 · 2 comments · Fixed by #1492
Milestone

Comments

@jedel1043
Copy link
Member

jedel1043 commented Aug 21, 2021

In the current state of the API, if we want to access a possibly missing argument of a function with a default value of undefined, we either clone to unwrap_or_default or create a new Value::Undefined and unwrap_of(&undefined).
I extracted two implementation examples from the codebase:

fn apply(this: &JsValue, args: &[JsValue], context: &mut Context) -> Result<JsValue> {
        if !this.is_function() {
            return context.throw_type_error(format!("{} is not a function", this.display()));
        }
        let this_arg = args.get(0).cloned().unwrap_or_default();
        let arg_array = args.get(1).cloned().unwrap_or_default();
        if arg_array.is_null_or_undefined() {
            // TODO?: 3.a. PrepareForTailCall
            return context.call(this, &this_arg, &[]);
        }
        let arg_list = arg_array.create_list_from_array_like(&[], context)?;
        // TODO?: 5. PrepareForTailCall
        context.call(this, &this_arg, &arg_list)
    }

The first alternative is not ideal, we are either copying primitive types or cloning an Rc just to drop it at the end of the function.

pub fn is_prototype_of(
        this: &JsValue,
        args: &[JsValue],
        context: &mut Context,
    ) -> Result<JsValue> {
        let undefined = JsValue::undefined();
        let mut v = args.get(0).unwrap_or(&undefined).clone();
        if !v.is_object() {
            return Ok(JsValue::new(false));
        }
        let o = JsValue::new(this.to_object(context)?);
        loop {
            v = Self::get_prototype_of(this, &[v], context)?;
            if v.is_null() {
                return Ok(JsValue::new(false));
            }
            if JsValue::same_value(&o, &v) {
                return Ok(JsValue::new(true));
            }
        }
    }

The second alternative is good, but it gets very repetitive having to always call get(n).unwrap_of(&undefined) and always creates an undefined variable at the beginning of a function.

We mainly use the former and sometimes the latter, but it would be ideal to be able to just call let this_arg = args.get_or_undefined(0);. The way I think we can optimally solve this is by creating a new Args trait, implementing it only for &[JsValue] and adding this new trait to the prelude, essentially making it public to the user.
Also, the function get_or_undefined should return a Cow<JsValue> to avoid cloning in case the argument exists and we just need the reference. Should it? Maybe the overhead of the indirection of Cow is greater than the overhead of cloning a JsValue. We would need to benchmark both options to determine the best.

Do you have any other solutions? I'd like to hear what you think.

@jedel1043 jedel1043 changed the title Refactor arguments of function objects to automatically return undefined Add get_or_undefined method to arguments of function objects Aug 21, 2021
@jedel1043 jedel1043 changed the title Add get_or_undefined method to arguments of function objects Add get_or_undefined method to arguments of builtin functions Aug 21, 2021
@raskad
Copy link
Member

raskad commented Aug 21, 2021

This looks great for usability. I think the trait should be named JsArgs or something similar, to make clear what it represents.

I am very interested in the performance implication of using Cow<JsValue>. As far as I understand it, Cow would add a layer of indirection to the JsValue. My expectation would be, that this actually makes the code slower. Cloning a primitive type or cloning an Rc pointer currently only happens one time. With Cow every use of the JsValue would have to go through the Cow pointer. But I'm not an expert on pointers so I may be completely off here.

@jedel1043
Copy link
Member Author

I am very interested in the performance implication of using Cow<JsValue>. As far as I understand it, Cow would add a layer of indirection to the JsValue. My expectation would be, that this actually makes the code slower. Cloning a primitive type or cloning an Rc pointer currently only happens one time. With Cow every use of the JsValue would have to go through the Cow pointer. But I'm not an expert on pointers so I may be completely off here.

Yeah, we would have to test both to determine the performance loss or possibly gain. However, we can implement this as it is, with a simple args.get(n).cloned().unwrap_or(JsValue::Default) and if we determine that Cow is faster, change the function internally. The main objective is to improve the usability, because this appears 60 times in the whole codebase.

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