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

Fix Accessors panics #902

Merged
merged 1 commit into from
Oct 20, 2020
Merged

Fix Accessors panics #902

merged 1 commit into from
Oct 20, 2020

Conversation

HalidOdat
Copy link
Member

It changes the following:

  • Accessors now return undefined.

@HalidOdat HalidOdat added the execution Issues or PRs related to code execution label Oct 20, 2020
@HalidOdat HalidOdat added this to the v0.11.0 milestone Oct 20, 2020
@github-actions
Copy link

Test262 conformance changes:

Test result master count PR count difference
Total 78,413 78,413 0
Passed 18,189 18,375 +186
Ignored 15,507 15,507 0
Failed 44,717 44,531 -186
Panics 13,519 12,260 -1,259
Conformance 23.20 23.43 +0.24%

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #902 into master will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #902      +/-   ##
==========================================
- Coverage   59.44%   59.41%   -0.03%     
==========================================
  Files         166      166              
  Lines       10474    10477       +3     
==========================================
- Hits         6226     6225       -1     
- Misses       4248     4252       +4     
Impacted Files Coverage Δ
boa/src/object/internal_methods.rs 57.14% <0.00%> (-0.83%) ⬇️
boa/src/value/mod.rs 74.10% <0.00%> (-0.49%) ⬇️

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 fbb1077...145aa2b. Read the comment docs.

@github-actions
Copy link

Benchmark for cad9698

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 436.6±18.23ns 354.9±26.96ns +23.02%
Arithmetic operations (Full) 285.6±10.22µs 257.5±20.07µs +10.91%
Array access (Execution) 9.5±0.43µs 8.8±0.66µs +7.95%
Array access (Full) 316.2±26.24µs 275.2±14.06µs +14.90%
Array creation (Execution) 2.7±0.19ms 2.8±0.18ms -3.57%
Array creation (Full) 3.3±0.15ms 2.9±0.24ms +13.79%
Array pop (Execution) 1093.9±45.80µs 1012.9±58.75µs +8.00%
Array pop (Full) 1594.1±54.20µs 1475.3±84.00µs +8.05%
Boolean Object Access (Execution) 5.3±0.22µs 4.6±0.28µs +15.22%
Boolean Object Access (Full) 316.0±20.91µs 277.8±21.39µs +13.75%
Clean js (Execution) 811.1±38.30µs 708.5±62.58µs +14.48%
Clean js (Full) 1192.3±89.50µs 992.4±60.31µs +20.14%
Clean js (Parser) 42.8±1.44µs 40.0±3.29µs +7.00%
Create Realm 520.8±31.50ns 485.6±35.38ns +7.25%
Dynamic Object Property Access (Execution) 6.2±0.46µs 5.9±0.35µs +5.08%
Dynamic Object Property Access (Full) 302.2±11.37µs 278.1±18.56µs +8.67%
Expression (Parser) 8.1±0.54µs 6.8±0.49µs +19.12%
Fibonacci (Execution) 1031.8±90.29µs 951.9±57.04µs +8.39%
Fibonacci (Full) 1300.2±78.80µs 1229.4±75.33µs +5.76%
For loop (Execution) 25.9±2.49µs 26.3±2.94µs -1.52%
For loop (Full) 327.7±15.44µs 292.7±17.26µs +11.96%
For loop (Parser) 19.2±1.40µs 19.0±1.55µs +1.05%
Goal Symbols (Parser) 14.2±1.06µs 12.6±0.71µs +12.70%
Hello World (Parser) 3.8±0.16µs 3.4±0.24µs +11.76%
Long file (Parser) 890.2±76.54ns 785.5±67.87ns +13.33%
Mini js (Execution) 686.3±57.55µs 628.8±41.18µs +9.14%
Mini js (Full) 1017.0±60.32µs 908.6±61.70µs +11.93%
Mini js (Parser) 36.8±2.53µs 34.4±1.89µs +6.98%
Number Object Access (Execution) 4.3±0.22µs 3.5±0.24µs +22.86%
Number Object Access (Full) 298.6±16.40µs 265.2±18.48µs +12.59%
Object Creation (Execution) 6.1±0.62µs 5.2±0.32µs +17.31%
Object Creation (Full) 314.0±19.31µs 275.6±14.41µs +13.93%
RegExp (Execution) 11.0±0.51µs 9.9±0.97µs +11.11%
RegExp (Full) 309.1±14.10µs 271.4±16.22µs +13.89%
RegExp Literal (Execution) 11.7±0.86µs 11.0±0.94µs +6.36%
RegExp Literal (Full) 313.0±11.18µs 272.8±16.22µs +14.74%
RegExp Literal Creation (Execution) 10.9±0.42µs 9.4±0.60µs +15.96%
RegExp Literal Creation (Full) 284.7±24.88µs 291.1±47.40µs -2.20%
Static Object Property Access (Execution) 5.9±0.40µs 5.6±0.27µs +5.36%
Static Object Property Access (Full) 301.8±10.81µs 268.2±16.71µs +12.53%
String Object Access (Execution) 7.9±0.34µs 7.1±0.42µs +11.27%
String Object Access (Full) 305.4±13.80µs 268.7±21.28µs +13.66%
String comparison (Execution) 7.0±0.43µs 6.8±0.60µs +2.94%
String comparison (Full) 308.6±18.60µs 269.3±16.93µs +14.59%
String concatenation (Execution) 6.3±0.27µs 5.5±0.33µs +14.55%
String concatenation (Full) 300.2±17.95µs 275.1±21.05µs +9.12%
String copy (Execution) 4.8±0.25µs 4.2±0.59µs +14.29%
String copy (Full) 292.0±25.12µs 267.7±21.77µs +9.08%
Symbols (Execution) 4.1±0.32µs 4.1±0.34µs 0.00%
Symbols (Full) 284.1±16.67µs 254.2±15.66µs +11.76%

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.

Thanks! Removing the panics should improve our conformance, and at at least we don't bring down the process executing the interpreter. Looks good to me, I'm a bit intrigued by the benchmarks, but I hope it's just statistical anomalies.

@HalidOdat
Copy link
Member Author

Looks good to me, I'm a bit intrigued by the benchmarks, but I hope it's just statistical anomalies.

That is very strange. You are right, it's probably a statistical anomaly since there is a performance regression in parse/lex benchmarks, I haven't changed anything in the parser/lexer.

@Razican
Copy link
Member

Razican commented Oct 20, 2020

Cool, let's merge then :D

@Razican Razican merged commit f3a293b into master Oct 20, 2020
@Razican Razican deleted the fix/accessor-panics branch October 20, 2020 17:53
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants