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

[Merged by Bors] - Preserve ints when executing int operations #1964

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Preserve ints when executing int operations #1964

wants to merge 2 commits into from

Conversation

jedel1043
Copy link
Member

This Pull Request fixes/closes #1962.

It changes the following:

  • When executing arithmetic operations on JsValues, try to use integer operations and fallback to f64 operations on error.
  • Adds tests for serde_json conversions from integer operations.

@github-actions
Copy link

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 88,428 88,428 0
Passed 43,986 43,986 0
Ignored 21,495 21,495 0
Failed 22,947 22,947 0
Panics 0 0 0
Conformance 49.74% 49.74% 0.00%

@jedel1043 jedel1043 added execution Issues or PRs related to code execution API labels Mar 21, 2022
@jedel1043 jedel1043 added this to the v0.15.0 milestone Mar 21, 2022
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #1964 (20f909e) into main (5fa1668) will increase coverage by 0.02%.
The diff coverage is 61.11%.

@@            Coverage Diff             @@
##             main    #1964      +/-   ##
==========================================
+ Coverage   45.87%   45.90%   +0.02%     
==========================================
  Files         206      206              
  Lines       17102    17116      +14     
==========================================
+ Hits         7846     7857      +11     
- Misses       9256     9259       +3
Impacted Files Coverage Δ
boa_engine/src/value/serde_json.rs 84.09% <ø> (ø)
boa_engine/src/value/operations.rs 39.17% <61.11%> (+1.09%) ⬆️
boa_engine/src/builtins/object/for_in_iterator.rs 91.42% <0.00%> (-2.86%) ⬇️
boa_engine/src/symbol.rs 31.81% <0.00%> (-1.52%) ⬇️
boa_engine/src/object/internal_methods/global.rs 31.25% <0.00%> (-0.90%) ⬇️
boa_engine/src/builtins/regexp/mod.rs 67.28% <0.00%> (-0.27%) ⬇️
boa_engine/src/builtins/generator/mod.rs 7.69% <0.00%> (-0.09%) ⬇️
boa_engine/src/builtins/set/mod.rs 87.80% <0.00%> (+0.15%) ⬆️
boa_engine/src/value/equality.rs 90.24% <0.00%> (+0.24%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fa1668...20f909e. Read the comment docs.

@github-actions
Copy link

Benchmark for 3f3f545

Click to view benchmark
Test Base PR %
Arithmetic operations (Compiler) 517.1±1.89ns 504.6±1.91ns -2.42%
Arithmetic operations (Execution) 1976.5±2.19ns 1975.4±3.26ns -0.06%
Arithmetic operations (Parser) 5.3±0.02µs 5.2±0.01µs -1.89%
Array access (Compiler) 1258.4±2.51ns 1268.2±2.42ns +0.78%
Array access (Execution) 9.7±0.03µs 9.7±0.03µs 0.00%
Array access (Parser) 11.4±0.05µs 11.5±0.03µs +0.88%
Array creation (Compiler) 1829.8±8.49ns 1831.8±6.70ns +0.11%
Array creation (Execution) 3.2±0.01ms 3.2±0.01ms 0.00%
Array creation (Parser) 12.9±0.03µs 12.9±0.01µs 0.00%
Array pop (Compiler) 3.8±0.01µs 3.8±0.01µs 0.00%
Array pop (Execution) 1385.6±2.73µs 1397.0±3.69µs +0.82%
Array pop (Parser) 129.4±1.36µs 129.3±0.07µs -0.08%
Boolean Object Access (Compiler) 1113.3±2.96ns 1106.9±2.82ns -0.57%
Boolean Object Access (Execution) 5.8±0.08µs 5.7±0.01µs -1.72%
Boolean Object Access (Parser) 13.8±0.02µs 13.9±0.02µs +0.72%
Clean js (Compiler) 3.4±0.01µs 3.3±0.02µs -2.94%
Clean js (Execution) 1087.4±8.82µs 1078.4±6.45µs -0.83%
Clean js (Parser) 27.8±0.04µs 27.9±0.09µs +0.36%
Create Realm 266.1±1.29ns 270.4±0.22ns +1.62%
Dynamic Object Property Access (Compiler) 1628.2±11.04ns 1607.1±1.74ns -1.30%
Dynamic Object Property Access (Execution) 6.7±0.05µs 6.7±0.02µs 0.00%
Dynamic Object Property Access (Parser) 10.1±0.07µs 10.2±0.02µs +0.99%
Fibonacci (Compiler) 2.3±0.01µs 2.3±0.00µs 0.00%
Fibonacci (Execution) 1773.9±4.06µs 1804.3±2.97µs +1.71%
Fibonacci (Parser) 15.5±0.04µs 15.5±0.02µs 0.00%
For loop (Compiler) 1944.4±3.86ns 1933.3±4.36ns -0.57%
For loop (Execution) 42.5±0.19µs 42.5±0.10µs 0.00%
For loop (Parser) 13.2±0.04µs 13.3±0.02µs +0.76%
Mini js (Compiler) 3.3±0.01µs 3.2±0.03µs -3.03%
Mini js (Execution) 997.6±9.10µs 992.6±10.35µs -0.50%
Mini js (Parser) 24.4±0.03µs 24.4±0.05µs 0.00%
Number Object Access (Compiler) 1044.2±3.87ns 1039.0±2.22ns -0.50%
Number Object Access (Execution) 4.6±0.02µs 4.5±0.02µs -2.17%
Number Object Access (Parser) 10.8±0.03µs 10.8±0.02µs 0.00%
Object Creation (Compiler) 1418.0±9.10ns 1388.2±6.15ns -2.10%
Object Creation (Execution) 6.0±0.02µs 6.0±0.13µs 0.00%
Object Creation (Parser) 8.8±0.03µs 8.9±0.02µs +1.14%
RegExp (Compiler) 1621.2±6.20ns 1611.9±1.81ns -0.57%
RegExp (Execution) 12.4±0.07µs 12.4±0.07µs 0.00%
RegExp (Parser) 9.8±0.09µs 9.8±0.01µs 0.00%
RegExp Creation (Compiler) 1405.1±7.77ns 1400.7±6.22ns -0.31%
RegExp Creation (Execution) 9.2±0.04µs 9.3±0.05µs +1.09%
RegExp Creation (Parser) 8.2±0.01µs 8.1±0.02µs -1.22%
RegExp Literal (Compiler) 1626.2±9.05ns 1612.5±1.22ns -0.84%
RegExp Literal (Execution) 12.5±0.08µs 12.4±0.08µs -0.80%
RegExp Literal (Parser) 7.9±0.02µs 8.0±0.02µs +1.27%
RegExp Literal Creation (Compiler) 1418.8±8.09ns 1388.3±7.20ns -2.15%
RegExp Literal Creation (Execution) 9.3±0.04µs 9.3±0.05µs 0.00%
RegExp Literal Creation (Parser) 6.2±0.02µs 6.3±0.02µs +1.61%
Static Object Property Access (Compiler) 1394.1±6.16ns 1388.3±4.68ns -0.42%
Static Object Property Access (Execution) 6.2±0.03µs 6.3±0.02µs +1.61%
Static Object Property Access (Parser) 9.5±0.02µs 9.6±0.02µs +1.05%
String Object Access (Compiler) 1494.0±9.74ns 1474.0±5.36ns -1.34%
String Object Access (Execution) 7.6±0.02µs 7.6±0.02µs 0.00%
String Object Access (Parser) 13.5±0.01µs 13.6±0.01µs +0.74%
String comparison (Compiler) 2.1±0.01µs 2.1±0.01µs 0.00%
String comparison (Execution) 5.8±0.02µs 5.8±0.02µs 0.00%
String comparison (Parser) 10.7±0.02µs 10.6±0.01µs -0.93%
String concatenation (Compiler) 1621.0±4.05ns 1614.7±12.00ns -0.39%
String concatenation (Execution) 5.2±0.01µs 5.3±0.02µs +1.92%
String concatenation (Parser) 7.3±0.02µs 7.3±0.01µs 0.00%
String copy (Compiler) 1291.4±2.54ns 1299.5±3.04ns +0.63%
String copy (Execution) 4.6±0.02µs 4.6±0.02µs 0.00%
String copy (Parser) 5.4±0.01µs 5.4±0.01µs 0.00%
Symbols (Compiler) 917.8±8.21ns 900.9±3.26ns -1.84%
Symbols (Execution) 4.5±0.01µs 4.4±0.01µs -2.22%
Symbols (Parser) 4.2±0.02µs 4.2±0.01µs 0.00%

HalidOdat
HalidOdat approved these changes Mar 22, 2022
View reviewed changes
Copy link
Member

@HalidOdat HalidOdat 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 to me! :)

RageKnify
RageKnify approved these changes Mar 22, 2022
View reviewed changes
Copy link
Contributor

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

bors r+

@jedel1043
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Mar 22, 2022
@jedel1043
Preserve ints when executing int operations (#1964)
e1c2e14 
This Pull Request fixes/closes #1962.

It changes the following:

- When executing arithmetic operations on `JsValue`s, try to use integer operations and fallback to `f64` operations on error.
- Adds tests for serde_json conversions from integer operations.
@bors
Copy link

bors bot commented Mar 22, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Preserve ints when executing int operations [Merged by Bors] - Preserve ints when executing int operations Mar 22, 2022
@bors bors bot closed this Mar 22, 2022
@bors bors bot deleted the int-ops branch March 22, 2022 19:13
@lastmjs lastmjs mentioned this pull request Mar 22, 2022
Closed
@Razican
Copy link
Member

Razican commented Mar 22, 2022

@jedel1043 does this solve #1147?

@jedel1043
Copy link
Member Author

@jedel1043 does this solve #1147?

Not really, this just optimizes arithmetic operations, and ++/-- are statements. Also, that issue is a bit more complex, since internally the increment operators use JsValue::to_number, I think.

@RageKnify RageKnify mentioned this pull request Mar 23, 2022
Closed
Razican pushed a commit that referenced this pull request Jun 8, 2022
@jedel1043 @Razican
Preserve ints when executing int operations (#1964)
7611276 
This Pull Request fixes/closes #1962.

It changes the following:

- When executing arithmetic operations on `JsValue`s, try to use integer operations and fallback to `f64` operations on error.
- Adds tests for serde_json conversions from integer operations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RageKnify RageKnify RageKnify approved these changes

@HalidOdat HalidOdat HalidOdat approved these changes

@Razican Razican Awaiting requested review from Razican

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

@raskad raskad Awaiting requested review from raskad

Assignees
No one assigned
Labels
API execution Issues or PRs related to code execution
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.

invalid type: floating point after math operations
4 participants
@jedel1043 @Razican @HalidOdat @RageKnify