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 GcObject to_json mutable borrow panic #1284

Merged
merged 1 commit into from
May 26, 2021
Merged

Conversation

0x7D2B
Copy link
Contributor

@0x7D2B 0x7D2B commented May 25, 2021

This Pull Request fixes #1059. The object was getting borrowed in to_json to iterate over its keys, preventing mutable borrows. The fix was to collect the keys first and to iterate over them. This will also make the test behave as expected as it will prevent properties added during iteration from being enumerated.

@0x7D2B 0x7D2B added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics execution Issues or PRs related to code execution rust Pull requests that update Rust code labels May 25, 2021
@0x7D2B 0x7D2B requested a review from Razican May 25, 2021 13:49
@github-actions
Copy link

Test262 conformance changes:

Test result master count PR count difference
Total 78,873 78,873 0
Passed 26,502 26,502 0
Ignored 15,604 15,604 0
Failed 36,767 36,767 0
Panics 27 24 -3
Conformance 33.60% 33.60% 0.00%

@Razican Razican added this to the v0.12.0 milestone May 25, 2021
@github-actions
Copy link

Benchmark for aad1c8e

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 334.3±13.72ns 324.1±14.96ns +3.15%
Arithmetic operations (Full) 249.5±9.22µs 241.8±10.37µs +3.18%
Array access (Execution) 5.9±0.22µs 5.5±0.28µs +7.27%
Array access (Full) 259.0±12.56µs 260.9±12.68µs -0.73%
Array creation (Execution) 2.7±0.10ms 2.7±0.07ms 0.00%
Array creation (Full) 2.9±0.12ms 2.9±0.12ms 0.00%
Array pop (Execution) 869.4±26.90µs 838.9±26.77µs +3.64%
Array pop (Full) 1288.9±49.07µs 1305.8±81.59µs -1.29%
Boolean Object Access (Execution) 4.9±0.18µs 4.7±0.23µs +4.26%
Boolean Object Access (Full) 267.8±9.61µs 261.6±11.71µs +2.37%
Clean js (Execution) 610.8±25.49µs 603.3±25.37µs +1.24%
Clean js (Full) 896.8±32.29µs 881.2±35.76µs +1.77%
Clean js (Parser) 36.0±1.96µs 37.3±1.65µs -3.49%
Create Realm 408.8±8.84ns 392.0±20.06ns +4.29%
Dynamic Object Property Access (Execution) 4.7±0.22µs 4.4±0.18µs +6.82%
Dynamic Object Property Access (Full) 256.3±12.23µs 272.2±12.36µs -5.84%
Expression (Parser) 6.7±0.30µs 6.7±0.36µs 0.00%
Fibonacci (Execution) 704.3±20.67µs 686.6±28.03µs +2.58%
Fibonacci (Full) 984.3±33.34µs 948.9±36.77µs +3.73%
For loop (Execution) 20.7±0.69µs 19.4±0.90µs +6.70%
For loop (Full) 272.0±13.48µs 275.4±14.01µs -1.23%
For loop (Parser) 18.5±0.78µs 18.2±0.66µs +1.65%
Goal Symbols (Parser) 12.8±0.50µs 12.7±0.54µs +0.79%
Hello World (Parser) 3.6±0.14µs 3.7±0.18µs -2.70%
Long file (Parser) 711.0±32.13ns 697.4±29.60ns +1.95%
Mini js (Execution) 542.5±21.38µs 541.3±26.86µs +0.22%
Mini js (Full) 853.7±29.87µs 854.4±25.84µs -0.08%
Mini js (Parser) 33.7±1.23µs 32.9±1.48µs +2.43%
Number Object Access (Execution) 3.9±0.13µs 3.8±0.14µs +2.63%
Number Object Access (Full) 255.9±10.33µs 257.5±11.40µs -0.62%
Object Creation (Execution) 4.1±0.15µs 3.9±0.18µs +5.13%
Object Creation (Full) 255.3±11.18µs 256.9±10.81µs -0.62%
RegExp (Execution) 11.2±0.51µs 10.7±0.51µs +4.67%
RegExp (Full) 269.1±9.57µs 271.0±11.83µs -0.70%
RegExp Literal (Execution) 10.9±0.61µs 11.2±0.33µs -2.68%
RegExp Literal (Full) 263.3±11.83µs 269.4±12.04µs -2.26%
RegExp Literal Creation (Execution) 9.8±0.41µs 9.5±0.40µs +3.16%
RegExp Literal Creation (Full) 272.7±10.31µs 264.1±10.71µs +3.26%
Static Object Property Access (Execution) 4.3±0.17µs 4.1±0.18µs +4.88%
Static Object Property Access (Full) 270.4±6.57µs 260.7±13.02µs +3.72%
String Object Access (Execution) 6.8±0.33µs 6.5±0.33µs +4.62%
String Object Access (Full) 272.1±9.67µs 261.4±10.71µs +4.09%
String comparison (Execution) 5.8±0.26µs 6.0±0.28µs -3.33%
String comparison (Full) 261.6±14.40µs 253.6±15.60µs +3.15%
String concatenation (Execution) 4.7±0.21µs 4.7±0.21µs 0.00%
String concatenation (Full) 251.3±11.16µs 259.6±11.71µs -3.20%
String copy (Execution) 3.6±0.15µs 3.6±0.13µs 0.00%
String copy (Full) 249.3±11.89µs 240.7±11.12µs +3.57%
Symbols (Execution) 3.2±0.11µs 3.0±0.16µs +6.67%
Symbols (Full) 241.7±10.83µs 245.4±13.11µs -1.51%

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.

This is good from my side. Hopefully performance won't suffer a lot.

@Razican Razican merged commit 81ef87b into master May 26, 2021
@0x7D2B 0x7D2B deleted the fix/to-json-panic branch May 26, 2021 14:31
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 rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Borrow panic in object internal methods
3 participants