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

Fixed a bunch of test262 panics (#817) #1046

Merged
merged 3 commits into from
Jan 8, 2021
Merged

Fixed a bunch of test262 panics (#817) #1046

merged 3 commits into from
Jan 8, 2021

Conversation

Razican
Copy link
Member

@Razican Razican commented Jan 7, 2021

This PR works towards a panic-free boa. It fixes:

  • Panics in RegExp
  • Wrong delete argument
  • parseInt() panics if radix was out of bounds.
  • Spec-compliant parseInt() function.

I also took the opportunity to update the dependencies and the test262 submodule, and fix a couple of clippy warnings in tests, along with a documentation link.

This also implements a spec-compliant `parseInt()` function.
@Razican Razican added bug Something isn't working test Issues and PRs related to the tests. builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution labels Jan 7, 2021
@Razican Razican added this to the v0.11.0 milestone Jan 7, 2021
@github-actions
Copy link

github-actions bot commented Jan 7, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,493 78,497 +4
Passed 24,443 24,515 +72
Ignored 15,585 15,585 0
Failed 38,465 38,397 -68
Panics 91 77 -14
Conformance 31.14 31.23 +0.09%

@codecov
Copy link

codecov bot commented Jan 7, 2021

Codecov Report

Merging #1046 (f17f718) into master (1a7e832) will increase coverage by 0.03%.
The diff coverage is 64.70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1046      +/-   ##
==========================================
+ Coverage   58.56%   58.60%   +0.03%     
==========================================
  Files         172      172              
  Lines       11920    11943      +23     
==========================================
+ Hits         6981     6999      +18     
- Misses       4939     4944       +5     
Impacted Files Coverage Δ
boa/src/context.rs 60.61% <0.00%> (ø)
boa/src/syntax/ast/node/operator/unary_op/mod.rs 56.41% <0.00%> (ø)
boa/src/builtins/regexp/mod.rs 66.09% <25.00%> (-1.16%) ⬇️
boa/src/value/mod.rs 73.43% <66.66%> (-0.25%) ⬇️
boa/src/builtins/number/mod.rs 64.73% <71.79%> (+1.62%) ⬆️

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 1a7e832...f17f718. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jan 7, 2021

Benchmark for af9976e

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 269.4±0.23ns 268.1±0.18ns +0.48%
Arithmetic operations (Full) 172.4±0.20µs 174.0±0.80µs -0.92%
Array access (Execution) 5.0±0.02µs 4.6±0.02µs +8.70%
Array access (Full) 188.0±0.85µs 189.5±0.23µs -0.79%
Array creation (Execution) 2.2±0.00ms 2.3±0.00ms -4.35%
Array creation (Full) 2.5±0.00ms 2.5±0.00ms 0.00%
Array pop (Execution) 730.4±1.36µs 727.1±1.37µs +0.45%
Array pop (Full) 1063.9±2.01µs 1054.4±9.62µs +0.90%
Boolean Object Access (Execution) 4.2±0.01µs 3.8±0.01µs +10.53%
Boolean Object Access (Full) 182.5±0.24µs 183.4±0.26µs -0.49%
Clean js (Execution) 531.4±2.26µs 511.0±2.83µs +3.99%
Clean js (Full) 724.5±4.55µs 724.8±3.87µs -0.04%
Clean js (Parser) 26.7±0.02µs 26.7±0.03µs 0.00%
Create Realm 445.6±0.37ns 362.0±2.91ns +23.09%
Dynamic Object Property Access (Execution) 4.4±0.03µs 3.8±0.01µs +15.79%
Dynamic Object Property Access (Full) 185.2±0.57µs 185.6±0.36µs -0.22%
Expression (Parser) 5.7±0.00µs 5.7±0.01µs 0.00%
Fibonacci (Execution) 659.2±0.93µs 637.1±0.70µs +3.47%
Fibonacci (Full) 837.2±1.56µs 842.3±0.94µs -0.61%
For loop (Execution) 17.8±0.05µs 17.3±0.07µs +2.89%
For loop (Full) 202.2±0.24µs 205.3±1.18µs -1.51%
For loop (Parser) 13.2±0.01µs 13.4±0.41µs -1.49%
Goal Symbols (Parser) 8.8±0.03µs 8.9±0.02µs -1.12%
Hello World (Parser) 2.3±0.01µs 2.4±0.02µs -4.17%
Long file (Parser) 609.8±0.65ns 620.8±0.88ns -1.77%
Mini js (Execution) 469.8±2.28µs 459.0±1.81µs +2.35%
Mini js (Full) 653.5±2.62µs 656.9±2.47µs -0.52%
Mini js (Parser) 23.5±0.02µs 23.7±0.02µs -0.84%
Number Object Access (Execution) 3.3±0.01µs 2.9±0.00µs +13.79%
Number Object Access (Full) 180.0±0.23µs 181.6±0.24µs -0.88%
Object Creation (Execution) 3.6±0.01µs 3.3±0.01µs +9.09%
Object Creation (Full) 206.1±0.60µs 183.2±0.22µs +12.50%
RegExp (Execution) 7.7±0.02µs 7.1±0.02µs +8.45%
RegExp (Full) 191.1±0.68µs 192.5±2.06µs -0.73%
RegExp Literal (Execution) 8.8±0.02µs 8.1±0.03µs +8.64%
RegExp Literal (Full) 190.3±0.68µs 216.7±0.49µs -12.18%
RegExp Literal Creation (Execution) 7.7±0.02µs 7.1±0.02µs +8.45%
RegExp Literal Creation (Full) 186.6±0.26µs 187.0±0.24µs -0.21%
Static Object Property Access (Execution) 3.9±0.02µs 3.5±0.01µs +11.43%
Static Object Property Access (Full) 184.1±0.37µs 184.3±0.60µs -0.11%
String Object Access (Execution) 6.0±0.01µs 5.2±0.01µs +15.38%
String Object Access (Full) 184.9±0.30µs 186.4±0.25µs -0.80%
String comparison (Execution) 5.2±0.01µs 4.9±0.01µs +6.12%
String comparison (Full) 188.9±1.90µs 213.3±0.38µs -11.44%
String concatenation (Execution) 4.3±0.01µs 3.9±0.01µs +10.26%
String concatenation (Full) 182.9±1.83µs 185.6±0.36µs -1.45%
String copy (Execution) 3.2±0.01µs 3.0±0.01µs +6.67%
String copy (Full) 178.8±0.40µs 181.8±0.65µs -1.65%
Symbols (Execution) 3.0±0.01µs 2.5±0.03µs +20.00%
Symbols (Full) 197.7±0.91µs 175.3±0.40µs +12.78%

Copy link
Member

@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.

A few of the comments citing the spec are really long, I think it'd be best to split into multiple lines.
The rest looks great.

boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/number/mod.rs Outdated Show resolved Hide resolved
@Razican
Copy link
Member Author

Razican commented Jan 7, 2021

A few of the comments citing the spec are really long, I think it'd be best to split into multiple lines.
The rest looks great.

Should be fixed now :)

@github-actions
Copy link

github-actions bot commented Jan 7, 2021

Benchmark for d5a8137

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 365.8±14.37ns 365.0±10.78ns +0.22%
Arithmetic operations (Full) 264.6±12.76µs 257.6±8.38µs +2.72%
Array access (Execution) 6.7±0.16µs 6.8±0.20µs -1.47%
Array access (Full) 278.7±13.47µs 276.7±6.58µs +0.72%
Array creation (Execution) 2.9±0.06ms 3.1±0.06ms -6.45%
Array creation (Full) 3.2±0.09ms 3.2±0.05ms 0.00%
Array pop (Execution) 928.0±29.67µs 956.2±17.57µs -2.95%
Array pop (Full) 1418.4±28.42µs 1399.1±22.13µs +1.38%
Boolean Object Access (Execution) 5.5±0.17µs 5.2±0.10µs +5.77%
Boolean Object Access (Full) 266.9±5.54µs 274.0±10.51µs -2.59%
Clean js (Execution) 707.1±17.66µs 744.5±22.55µs -5.02%
Clean js (Full) 1020.3±33.80µs 1024.0±33.29µs -0.36%
Clean js (Parser) 36.9±1.68µs 36.8±1.21µs +0.27%
Create Realm 488.5±12.01ns 505.2±20.51ns -3.31%
Dynamic Object Property Access (Execution) 5.5±0.23µs 5.6±0.12µs -1.79%
Dynamic Object Property Access (Full) 269.2±6.39µs 279.2±13.47µs -3.58%
Expression (Parser) 6.8±0.13µs 6.9±0.17µs -1.45%
Fibonacci (Execution) 963.5±38.27µs 974.3±26.69µs -1.11%
Fibonacci (Full) 1260.7±33.44µs 1233.1±48.90µs +2.24%
For loop (Execution) 25.1±0.79µs 24.7±0.98µs +1.62%
For loop (Full) 301.3±14.24µs 303.7±5.56µs -0.79%
For loop (Parser) 18.0±0.46µs 18.3±0.32µs -1.64%
Goal Symbols (Parser) 12.2±0.45µs 12.2±0.32µs 0.00%
Hello World (Parser) 3.2±0.06µs 3.2±0.07µs 0.00%
Long file (Parser) 794.7±15.69ns 800.3±26.56ns -0.70%
Mini js (Execution) 627.1±18.58µs 634.9±9.77µs -1.23%
Mini js (Full) 925.9±19.23µs 927.1±15.40µs -0.13%
Mini js (Parser) 32.0±0.48µs 32.4±1.03µs -1.23%
Number Object Access (Execution) 4.1±0.07µs 4.2±0.17µs -2.38%
Number Object Access (Full) 270.2±5.88µs 267.5±7.37µs +1.01%
Object Creation (Execution) 4.7±0.15µs 4.9±0.16µs -4.08%
Object Creation (Full) 266.3±6.13µs 269.3±12.35µs -1.11%
RegExp (Execution) 10.1±0.34µs 10.2±0.70µs -0.98%
RegExp (Full) 292.7±17.05µs 285.4±9.36µs +2.56%
RegExp Literal (Execution) 11.5±0.36µs 11.6±0.24µs -0.86%
RegExp Literal (Full) 289.0±10.02µs 285.1±8.97µs +1.37%
RegExp Literal Creation (Execution) 10.0±0.30µs 10.1±0.60µs -0.99%
RegExp Literal Creation (Full) 271.5±6.18µs 290.8±13.62µs -6.64%
Static Object Property Access (Execution) 5.0±0.19µs 5.1±0.16µs -1.96%
Static Object Property Access (Full) 274.8±4.83µs 272.5±10.04µs +0.84%
String Object Access (Execution) 7.5±0.17µs 7.5±0.18µs 0.00%
String Object Access (Full) 273.0±9.22µs 273.4±11.29µs -0.15%
String comparison (Execution) 6.8±0.16µs 7.0±0.69µs -2.86%
String comparison (Full) 269.8±4.92µs 276.6±16.14µs -2.46%
String concatenation (Execution) 5.6±0.29µs 5.7±0.18µs -1.75%
String concatenation (Full) 275.4±6.02µs 271.8±18.12µs +1.32%
String copy (Execution) 4.3±0.15µs 4.3±0.15µs 0.00%
String copy (Full) 260.5±6.15µs 269.0±21.01µs -3.16%
Symbols (Execution) 3.9±0.25µs 3.9±0.15µs 0.00%
Symbols (Full) 259.1±8.92µs 263.1±28.11µs -1.52%

@Razican Razican merged commit 624d03d into master Jan 8, 2021
@RageKnify RageKnify deleted the fix_more_panics branch January 8, 2021 22:42
@RageKnify RageKnify mentioned this pull request Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants