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

Implement new get_or_undefined method for [JsValue] #1492

Merged
merged 5 commits into from
Sep 6, 2021

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Aug 22, 2021

This Pull Request fixes/closes #1490.

It changes the following:

  • Adds a new trait JsArgs to add new methods to [JsValue]
  • Implements a new method get_or_undefined for [JsValue]
  • Replaces calls to args.get(n).cloned().unwrap_or_default() with args.get_or_undefined()

This should improve the usability of the api by not having to unwrap_or_default everytime we want an argument inside a builtin function.

@jedel1043 jedel1043 changed the title Implement new get_or_undefined method for [JsValue] (Clone version) Implement new get_or_undefined method for [JsValue] (Clone version) Aug 22, 2021
@jedel1043 jedel1043 changed the title Implement new get_or_undefined method for [JsValue] (Clone version) Implement new get_or_undefined method for [JsValue] Aug 22, 2021
@jedel1043 jedel1043 marked this pull request as ready for review August 22, 2021 17:55
@Razican
Copy link
Member

Razican commented Aug 23, 2021

Hmmm I'm not sure if the extra trait and code improves the ergonomic enough to stoping using well known standard library APIs and adding a new trait.

I would like to know the thoughts of more people.

@RageKnify
Copy link
Member

If we are to do something like this I think returning a reference would be better, that way we would be reducing the number of clones.
To get the reference to undefined it might work to have a constant? Not sure if it's possible, but if so I think it'd be a nice solution.
We'd have const undefined: Value = Value::Undefined; in the trait's file and then we can use a reference to it in the method.

@jedel1043
Copy link
Member Author

Hmmm I'm not sure if the extra trait and code improves the ergonomic enough to stoping using well known standard library APIs and adding a new trait.

Completely understandable. I just wanted to point out that you can still use get as always without any problem; there are a lot of uses of get without defaulting to undefined. This is just a utility function to simplify a common action that appears 60+ times in the codebase.

@jedel1043
Copy link
Member Author

If we are to do something like this I think returning a reference would be better, that way we would be reducing the number of clones.
To get the reference to undefined it might work to have a constant? Not sure if it's possible, but if so I think it'd be a nice solution.
We'd have const undefined: Value = Value::Undefined; in the trait's file and then we can use a reference to it in the method.

Let me try this. I tried to use a Cow but the performance was worse than cloning, so i opted for the clone. A const if its possible or a static if it isn't should do the trick.

@jedel1043 jedel1043 force-pushed the args_auto_undefined_clone branch 4 times, most recently from 45340ce to 5c96fc0 Compare August 24, 2021 21:18
@Razican
Copy link
Member

Razican commented Aug 25, 2021

┌─────────┬──────────────────────────────────────────────┬────────────────────┬─────────────────────┬────────────┐
│ (index) │                     name                     │  changesDuration   │   masterDuration    │ difference │
├─────────┼──────────────────────────────────────────────┼────────────────────┼─────────────────────┼────────────┤
│    0    │     'Arithmetic operations (Execution)'      │   '228.6±0.33ns'   │ '**221.5±0.46ns**'  │   '+3.0'   │
│    1    │        'Arithmetic operations (Full)'        │ '**263.7±0.46µs**' │   '266.1±0.49µs'    │  '-0.99'   │
│    2    │          'Array access (Execution)'          │  '**5.0±0.02µs**'  │    '5.8±0.02µs'     │   '-13'    │
│    3    │            'Array access (Full)'             │   '288.5±0.49µs'   │   '287.3±0.50µs'    │   '0.0'    │
│    4    │         'Array creation (Execution)'         │  '**2.2±0.01ms**'  │    '2.7±0.00ms'     │   '-18'    │
│    5    │           'Array creation (Full)'            │    '2.9±0.00ms'    │  '**2.7±0.00ms**'   │   '+10'    │
│    6    │           'Array pop (Execution)'            │ '**816.0±2.49µs**' │   '863.8±2.42µs'    │   '-5.7'   │
│    7    │              'Array pop (Full)'              │  '1280.0±1.12µs'   │ '**1213.2±1.42µs**' │   '+6.0'   │
│    8    │     'Boolean Object Access (Execution)'      │  '**4.6±0.01µs**'  │    '4.6±0.01µs'     │  '-0.99'   │
│    9    │        'Boolean Object Access (Full)'        │   '282.8±0.35µs'   │   '281.7±0.36µs'    │   '0.0'    │
│   10    │            'Clean js (Execution)'            │ '**624.6±3.57µs**' │   '637.9±3.52µs'    │   '-2.0'   │
│   11    │              'Clean js (Full)'               │   '939.3±3.08µs'   │ '**930.7±3.05µs**'  │   '+1.0'   │
│   12    │             'Clean js (Parser)'              │ '**31.0±0.04µs**'  │    '31.3±0.15µs'    │  '-0.99'   │
│   13    │                'Create Realm'                │ '**312.4±0.51ns**' │   '349.6±1.77ns'    │   '-11'    │
│   14    │ 'Dynamic Object Property Access (Execution)' │  '**3.9±0.02µs**'  │    '4.5±0.01µs'     │   '-13'    │
│   15    │   'Dynamic Object Property Access (Full)'    │   '285.8±0.44µs'   │   '286.2±1.20µs'    │   '0.0'    │
│   16    │            'Expression (Parser)'             │    '5.2±0.01µs'    │    '5.2±0.02µs'     │   '0.0'    │
│   17    │           'Fibonacci (Execution)'            │   '687.8±2.98µs'   │ '**677.5±0.67µs**'  │   '+2.0'   │
│   18    │              'Fibonacci (Full)'              │   '963.8±1.72µs'   │   '967.6±2.30µs'    │   '0.0'    │
│   19    │            'For loop (Execution)'            │ '**15.1±0.06µs**'  │    '17.0±0.07µs'    │   '-12'    │
│   20    │              'For loop (Full)'               │ '**290.3±1.53µs**' │   '294.1±0.37µs'    │  '-0.99'   │
│   21    │             'For loop (Parser)'              │   '14.7±0.05µs'    │    '14.8±0.01µs'    │   '0.0'    │
│   22    │           'Goal Symbols (Parser)'            │ '**10.6±0.01µs**'  │    '10.6±0.01µs'    │  '-0.99'   │
│   23    │            'Hello World (Parser)'            │    '3.0±0.02µs'    │    '3.0±0.01µs'     │   '0.0'    │
│   24    │             'Long file (Parser)'             │   '729.1±1.47ns'   │   '730.3±9.76ns'    │   '0.0'    │
│   25    │            'Mini js (Execution)'             │ '**572.2±4.35µs**' │   '585.1±3.88µs'    │   '-2.0'   │
│   26    │               'Mini js (Full)'               │   '885.2±2.57µs'   │ '**873.6±2.65µs**'  │   '+1.0'   │
│   27    │              'Mini js (Parser)'              │ '**27.4±0.03µs**'  │    '27.6±0.03µs'    │  '-0.99'   │
│   28    │      'Number Object Access (Execution)'      │    '3.7±0.01µs'    │    '3.6±0.01µs'     │   '0.0'    │
│   29    │        'Number Object Access (Full)'         │ '**276.3±0.34µs**' │   '277.9±0.43µs'    │  '-0.99'   │
│   30    │        'Object Creation (Execution)'         │  '**4.0±0.01µs**'  │    '4.0±0.02µs'     │  '-0.99'   │
│   31    │           'Object Creation (Full)'           │   '279.6±0.26µs'   │   '280.5±0.48µs'    │   '0.0'    │
│   32    │             'RegExp (Execution)'             │   '11.0±0.06µs'    │    '10.9±0.06µs'    │   '0.0'    │
│   33    │               'RegExp (Full)'                │ '**285.2±0.35µs**' │   '287.4±0.32µs'    │  '-0.99'   │
│   34    │         'RegExp Literal (Execution)'         │   '11.0±0.06µs'    │    '11.0±0.06µs'    │   '0.0'    │
│   35    │           'RegExp Literal (Full)'            │ '**292.2±1.80µs**' │   '294.3±0.30µs'    │  '-0.99'   │
│   36    │    'RegExp Literal Creation (Execution)'     │  '**7.2±0.03µs**'  │    '8.0±0.02µs'     │   '-11'    │
│   37    │       'RegExp Literal Creation (Full)'       │   '284.9±0.63µs'   │   '284.8±0.34µs'    │   '0.0'    │
│   38    │ 'Static Object Property Access (Execution)'  │  '**3.7±0.01µs**'  │    '4.2±0.02µs'     │   '-12'    │
│   39    │    'Static Object Property Access (Full)'    │   '281.3±0.53µs'   │   '281.0±0.50µs'    │   '0.0'    │
│   40    │      'String Object Access (Execution)'      │    '6.1±0.01µs'    │    '6.1±0.03µs'     │   '0.0'    │
│   41    │        'String Object Access (Full)'         │   '285.1±0.55µs'   │   '283.7±0.38µs'    │   '0.0'    │
│   42    │       'String comparison (Execution)'        │  '**5.3±0.01µs**'  │    '5.8±0.03µs'     │   '-9.9'   │
│   43    │          'String comparison (Full)'          │ '**281.2±1.45µs**' │   '287.0±2.68µs'    │   '-2.0'   │
│   44    │      'String concatenation (Execution)'      │  '**4.0±0.02µs**'  │    '4.5±0.02µs'     │   '-11'    │
│   45    │        'String concatenation (Full)'         │ '**275.9±2.82µs**' │   '280.3±0.44µs'    │   '-2.0'   │
│   46    │          'String copy (Execution)'           │  '**3.1±0.01µs**'  │    '3.6±0.01µs'     │   '-12'    │
│   47    │             'String copy (Full)'             │ '**270.9±0.35µs**' │   '273.5±0.44µs'    │  '-0.99'   │
│   48    │            'Symbols (Execution)'             │  '**2.7±0.03µs**'  │    '3.0±0.01µs'     │   '-9.1'   │
│   49    │               'Symbols (Full)'               │ '**261.7±0.55µs**' │   '264.0±2.52µs'    │  '-0.99'   │
│   50    │                      ''                      │     undefined      │      undefined      │   '+NaN'   │
└─────────┴──────────────────────────────────────────────┴────────────────────┴─────────────────────┴────────────┘

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

I'm OK with the changes :) I though we might have a performance improvement, but not really.

@jedel1043
Copy link
Member Author

I'm OK with the changes :) I though we might have a performance improvement, but not really.

Still, there's a lot of small improvements in performance. The last version using clone was a lot slower.

@Razican Razican added API enhancement New feature or request labels Aug 25, 2021
@Razican Razican added this to the v0.13.0 milestone Aug 25, 2021
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Looks good!

@raskad raskad merged commit 2a8d343 into boa-dev:master Sep 6, 2021
@jedel1043 jedel1043 deleted the args_auto_undefined_clone branch September 6, 2021 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add get_or_undefined method to arguments of builtin functions
4 participants