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

[Merged by Bors] - Add integer type to fast path of to_property_key #2261

Closed
wants to merge 4 commits into from

Conversation

tunz
Copy link
Contributor

@tunz tunz commented Sep 3, 2022

Skip to_string for integer type primitives in to_property_key. It's unnecessary to convert the integer value to string and convert back to Index(u32) type.

In this example code, it improves around 10% of runtime.

let arr = [1,2,3,4,5];
for (let i = 0; i < 10000000; i++) {
  arr[0] = 123;
}

Before: 6.24s
After: 5.38s

@tunz tunz changed the title Add integer to fast path of to_property_key Add integer type to fast path of to_property_key Sep 3, 2022
@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #2261 (12302d8) into main (71daf2a) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2261      +/-   ##
==========================================
+ Coverage   41.31%   41.34%   +0.02%     
==========================================
  Files         234      234              
  Lines       22019    22023       +4     
==========================================
+ Hits         9097     9105       +8     
+ Misses      12922    12918       -4     
Impacted Files Coverage Δ
boa_engine/src/value/mod.rs 53.19% <100.00%> (+1.03%) ⬆️
boa_engine/src/bytecompiler/mod.rs 28.77% <0.00%> (+0.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jedel1043 jedel1043 added execution Issues or PRs related to code execution Internal Category for changelog labels Sep 3, 2022
@jedel1043 jedel1043 added this to the v0.16.0 milestone Sep 3, 2022
@raskad
Copy link
Member

raskad commented Sep 3, 2022

Very nice optimization. I think we could event optimize this further if I read the spec right. The slow path using ToPrimitive is only being used if the input is an object right? So we could use the fast path with the direct ToString conversion for every other JsValue type. If you want you can add that to this PR.

@tunz
Copy link
Contributor Author

tunz commented Sep 3, 2022

Updated. There was no notable performance improvement though since JsValue.to_primitive already returns just a cloned itself when it's not an object type. So, I could see that rust function call overhead is gone, but it's quite small compared to to_string call overhead.

@Razican Razican added run-benchmark Label used to run banchmarks on PRs performance Performance related changes and issues labels Sep 4, 2022
@Razican
Copy link
Member

Razican commented Sep 5, 2022

Benchmarks
┌─────────┬──────────────────────────────────────────────┬───────────────────────┬──────────────────────┬────────────┐
│ (index) │                     name                     │     baseDuration      │   changesDuration    │ difference │
├─────────┼──────────────────────────────────────────────┼───────────────────────┼──────────────────────┼────────────┤
│    0    │      'Arithmetic operations (Compiler)'      │    '582.0±24.23ns'    │   '581.4±27.33ns'    │   '0.0'    │
│    1    │     'Arithmetic operations (Execution)'      │    '797.4±28.26ns'    │ '**740.2±31.28ns**'  │   '-7.4'   │
│    2    │       'Arithmetic operations (Parser)'       │     '6.4±0.28µs'      │     '6.4±0.30µs'     │   '0.0'    │
│    3    │          'Array access (Compiler)'           │   '1484.2±56.38ns'    │ '**1459.4±70.17ns**' │   '-2.0'   │
│    4    │          'Array access (Execution)'          │   '**7.4±0.41µs**'    │     '7.7±0.40µs'     │   '+3.0'   │
│    5    │           'Array access (Parser)'            │   '**13.0±0.69µs**'   │    '13.3±0.59µs'     │   '+3.0'   │
│    6    │         'Array creation (Compiler)'          │   '**2.3±0.10µs**'    │     '2.4±0.10µs'     │   '+2.0'   │
│    7    │         'Array creation (Execution)'         │   '1188.3±58.61µs'    │ '**1173.8±45.82µs**' │  '-0.99'   │
│    8    │          'Array creation (Parser)'           │     '15.7±0.69µs'     │  '**15.5±0.62µs**'   │  '-0.99'   │
│    9    │            'Array pop (Compiler)'            │   '**4.2±0.22µs**'    │     '4.7±0.19µs'     │   '+11'    │
│   10    │           'Array pop (Execution)'            │    '632.2±22.82µs'    │ '**622.1±25.59µs**'  │   '-2.0'   │
│   11    │             'Array pop (Parser)'             │    '153.9±5.96µs'     │  '**152.6±6.84µs**'  │  '-0.99'   │
│   12    │      'Boolean Object Access (Compiler)'      │   '1106.4±55.68ns'    │ '**1063.6±41.61ns**' │   '-3.8'   │
│   13    │     'Boolean Object Access (Execution)'      │     '4.6±0.18µs'      │   '**4.6±0.18µs**'   │   '-2.0'   │
│   14    │       'Boolean Object Access (Parser)'       │     '16.3±0.63µs'     │  '**15.9±0.56µs**'   │   '-2.9'   │
│   15    │            'Clean js (Compiler)'             │   '**4.8±0.22µs**'    │     '5.1±0.20µs'     │   '+5.0'   │
│   16    │            'Clean js (Execution)'            │    '677.3±25.34µs'    │ '**654.4±25.76µs**'  │   '-3.8'   │
│   17    │             'Clean js (Parser)'              │   '**31.9±1.75µs**'   │    '32.1±1.60µs'     │   '+1.0'   │
│   18    │                'Create Realm'                │  '**250.7±12.35ns**'  │    '271.5±6.97ns'    │   '+8.0'   │
│   19    │ 'Dynamic Object Property Access (Compiler)'  │ '**1730.3±110.15ns**' │   '1764.2±79.23ns'   │   '+2.0'   │
│   20    │ 'Dynamic Object Property Access (Execution)' │   '**5.0±0.22µs**'    │     '5.3±0.19µs'     │   '+6.0'   │
│   21    │  'Dynamic Object Property Access (Parser)'   │     '12.2±0.47µs'     │  '**12.0±0.51µs**'   │   '-2.0'   │
│   22    │            'Fibonacci (Compiler)'            │     '2.8±0.12µs'      │     '2.8±0.16µs'     │   '0.0'    │
│   23    │           'Fibonacci (Execution)'            │ '**1060.3±46.87µs**'  │   '1098.0±47.93µs'   │   '+4.0'   │
│   24    │             'Fibonacci (Parser)'             │   '**18.0±0.98µs**'   │    '19.1±0.72µs'     │   '+6.0'   │
│   25    │            'For loop (Compiler)'             │     '2.6±0.15µs'      │   '**2.6±0.13µs**'   │  '-0.99'   │
│   26    │            'For loop (Execution)'            │   '**17.7±0.90µs**'   │    '17.9±0.66µs'     │   '+1.0'   │
│   27    │             'For loop (Parser)'              │     '16.3±0.81µs'     │    '16.3±0.79µs'     │   '0.0'    │
│   28    │             'Mini js (Compiler)'             │     '4.3±0.20µs'      │     '4.3±0.23µs'     │   '0.0'    │
│   29    │            'Mini js (Execution)'             │    '591.4±26.26µs'    │   '591.2±23.97µs'    │   '0.0'    │
│   30    │              'Mini js (Parser)'              │     '28.2±1.40µs'     │    '28.1±1.38µs'     │   '0.0'    │
│   31    │      'Number Object Access (Compiler)'       │   '1044.2±52.16ns'    │ '**1030.6±40.41ns**' │  '-0.99'   │
│   32    │      'Number Object Access (Execution)'      │   '**3.4±0.19µs**'    │     '3.6±0.15µs'     │   '+4.0'   │
│   33    │       'Number Object Access (Parser)'        │     '12.8±0.55µs'     │  '**12.3±0.46µs**'   │   '-3.8'   │
│   34    │         'Object Creation (Compiler)'         │ '**1600.5±85.36ns**'  │   '1610.6±70.80ns'   │   '+1.0'   │
│   35    │        'Object Creation (Execution)'         │     '5.0±0.16µs'      │   '**4.8±0.22µs**'   │   '-3.8'   │
│   36    │          'Object Creation (Parser)'          │     '10.7±0.37µs'     │  '**10.4±0.43µs**'   │   '-2.9'   │
│   37    │             'RegExp (Compiler)'              │ '**1736.3±92.44ns**'  │   '1761.1±72.55ns'   │   '+1.0'   │
│   38    │             'RegExp (Execution)'             │   '**12.2±0.61µs**'   │    '12.5±0.41µs'     │   '+2.0'   │
│   39    │              'RegExp (Parser)'               │     '11.6±0.58µs'     │  '**11.4±0.40µs**'   │   '-2.0'   │
│   40    │         'RegExp Creation (Compiler)'         │ '**1502.8±73.66ns**'  │  '1603.4±108.97ns'   │   '+7.0'   │
│   41    │        'RegExp Creation (Execution)'         │   '**8.8±0.44µs**'    │     '9.5±0.46µs'     │   '+8.0'   │
│   42    │          'RegExp Creation (Parser)'          │   '**9.5±0.40µs**'    │     '9.9±0.25µs'     │   '+4.0'   │
│   43    │         'RegExp Literal (Compiler)'          │   '1796.3±67.51ns'    │ '**1758.1±80.26ns**' │   '-2.0'   │
│   44    │         'RegExp Literal (Execution)'         │   '**11.9±0.54µs**'   │    '12.2±0.52µs'     │   '+3.0'   │
│   45    │          'RegExp Literal (Parser)'           │     '9.2±0.43µs'      │     '9.2±0.37µs'     │   '0.0'    │
│   46    │     'RegExp Literal Creation (Compiler)'     │   '1568.8±72.29ns'    │ '**1552.9±68.28ns**' │  '-0.99'   │
│   47    │    'RegExp Literal Creation (Execution)'     │   '**8.6±0.38µs**'    │     '9.2±0.45µs'     │   '+8.0'   │
│   48    │      'RegExp Literal Creation (Parser)'      │     '7.2±0.31µs'      │   '**7.1±0.36µs**'   │  '-0.99'   │
│   49    │  'Static Object Property Access (Compiler)'  │ '**1559.1±74.10ns**'  │   '1602.0±67.95ns'   │   '+3.0'   │
│   50    │ 'Static Object Property Access (Execution)'  │   '**4.8±0.25µs**'    │     '5.1±0.21µs'     │   '+4.0'   │
│   51    │   'Static Object Property Access (Parser)'   │     '11.7±0.37µs'     │  '**11.0±0.45µs**'   │   '-5.7'   │
│   52    │      'String Object Access (Compiler)'       │   '1430.3±49.90ns'    │ '**1378.1±61.42ns**' │   '-3.8'   │
│   53    │      'String Object Access (Execution)'      │     '6.7±0.28µs'      │   '**6.5±0.27µs**'   │   '-2.9'   │
│   54    │       'String Object Access (Parser)'        │     '15.7±0.63µs'     │  '**15.5±0.66µs**'   │  '-0.99'   │
│   55    │        'String comparison (Compiler)'        │     '2.4±0.23µs'      │   '**2.4±0.09µs**'   │  '-0.99'   │
│   56    │       'String comparison (Execution)'        │   '**4.3±0.19µs**'    │     '4.4±0.25µs'     │   '+3.0'   │
│   57    │         'String comparison (Parser)'         │   '**12.2±0.48µs**'   │    '12.5±0.48µs'     │   '+3.0'   │
│   58    │      'String concatenation (Compiler)'       │ '**1810.0±104.08ns**' │   '1824.3±83.23ns'   │   '+1.0'   │
│   59    │      'String concatenation (Execution)'      │   '**4.0±0.17µs**'    │     '4.1±0.17µs'     │   '+1.0'   │
│   60    │       'String concatenation (Parser)'        │     '8.6±0.40µs'      │   '**8.4±0.37µs**'   │   '-2.0'   │
│   61    │           'String copy (Compiler)'           │ '**1428.4±54.00ns**'  │   '1463.9±70.01ns'   │   '+2.0'   │
│   62    │          'String copy (Execution)'           │   '**3.7±0.21µs**'    │     '3.8±0.14µs'     │   '+5.0'   │
│   63    │            'String copy (Parser)'            │     '6.5±0.27µs'      │   '**6.4±0.31µs**'   │  '-0.99'   │
│   64    │             'Symbols (Compiler)'             │ '**1075.1±53.97ns**'  │   '1095.9±46.67ns'   │   '+2.0'   │
│   65    │            'Symbols (Execution)'             │   '**3.8±0.16µs**'    │     '3.9±0.19µs'     │   '+2.0'   │
│   66    │              'Symbols (Parser)'              │   '**4.8±0.24µs**'    │     '5.0±0.19µs'     │   '+5.0'   │
│   67    │                      ''                      │       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 was unable to reproduce performance gains in my laptop or on GitHub actions, but I guess it's reasonable. LGTM :) thanks!

@tunz
Copy link
Contributor Author

tunz commented Sep 5, 2022

Thanks for review :)
Interesting. maybe toString is not so expensive for some environment. I was testing on my MacOS arm64 machine.

@Razican
Copy link
Member

Razican commented Sep 5, 2022

Thanks for review :)
Interesting. maybe toString is not so expensive for some environment. I was testing on my MacOS arm64 machine.

Interesting indeed, as I have the same hardware. But might have been that I had other stuff running at the same time. No worries, the optimization seems very reasonable :)

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Very good work! And the new test is really appreciated ☺️

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 5, 2022
Skip `to_string` for integer type primitives in `to_property_key`. It's unnecessary to convert the integer value to string and convert back to `Index(u32)` type.

In this example code, it improves around 10% of runtime.
```js
let arr = [1,2,3,4,5];
for (let i = 0; i < 10000000; i++) {
  arr[0] = 123;
}
```

Before: 6.24s
After: 5.38s
@bors
Copy link

bors bot commented Sep 5, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add integer type to fast path of to_property_key [Merged by Bors] - Add integer type to fast path of to_property_key Sep 5, 2022
@bors bors bot closed this Sep 5, 2022
@tunz tunz deleted the tunz/int-prop-key-fast-path branch September 12, 2022 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Issues or PRs related to code execution Internal Category for changelog performance Performance related changes and issues run-benchmark Label used to run banchmarks on PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants