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

Refactor: Change Realm::global_object field from Value to GcObject #1067

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

RageKnify
Copy link
Contributor

This Pull Request works on #577.

It changes the following:

  • Change Realm::global_object field from Value to GcObject
  • Change make_builin_fn's parent argument from &Value to &GcObject
  • Change iterators' create_prototype functions to return GcObject rather than Value

@RageKnify RageKnify added the execution Issues or PRs related to code execution label Jan 12, 2021
@RageKnify RageKnify force-pushed the refactor/global_gcobject branch from 916fa00 to 0abe33c Compare January 12, 2021 23:44
@github-actions
Copy link

github-actions bot commented Jan 12, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,497 78,497 0
Passed 24,799 24,799 0
Ignored 15,587 15,587 0
Failed 38,111 38,111 0
Panics 26 26 0
Conformance 31.59 31.59 0.00%

@codecov
Copy link

codecov bot commented Jan 12, 2021

Codecov Report

Merging #1067 (66ad7f6) into master (f62a77d) will increase coverage by 0.01%.
The diff coverage is 89.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1067      +/-   ##
==========================================
+ Coverage   58.69%   58.71%   +0.01%     
==========================================
  Files         176      176              
  Lines       12389    12385       -4     
==========================================
  Hits         7272     7272              
+ Misses       5117     5113       -4     
Impacted Files Coverage Δ
boa/src/value/mod.rs 73.68% <ø> (+0.24%) ⬆️
boa/src/context.rs 61.32% <20.00%> (+0.71%) ⬆️
boa/src/realm.rs 87.50% <87.50%> (-5.36%) ⬇️
boa/src/builtins/array/array_iterator.rs 78.72% <100.00%> (-0.45%) ⬇️
boa/src/builtins/function/mod.rs 69.64% <100.00%> (ø)
boa/src/builtins/global_this/mod.rs 71.42% <100.00%> (ø)
boa/src/builtins/iterable/mod.rs 91.66% <100.00%> (-0.34%) ⬇️
boa/src/builtins/map/map_iterator.rs 69.64% <100.00%> (-0.54%) ⬇️
boa/src/builtins/map/mod.rs 72.36% <100.00%> (+0.36%) ⬆️
boa/src/builtins/mod.rs 21.21% <100.00%> (-2.32%) ⬇️
... and 6 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 f62a77d...66ad7f6. Read the comment docs.

@github-actions
Copy link

Benchmark for edd499f

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 387.0±16.49ns 399.3±17.45ns -3.08%
Arithmetic operations (Full) 265.4±10.35µs 262.6±9.88µs +1.07%
Array access (Execution) 7.0±0.26µs 7.1±0.22µs -1.41%
Array access (Full) 298.3±14.38µs 299.1±11.10µs -0.27%
Array creation (Execution) 2.9±0.10ms 3.0±0.13ms -3.33%
Array creation (Full) 3.3±0.16ms 3.3±0.21ms 0.00%
Array pop (Execution) 945.5±27.71µs 961.8±27.85µs -1.69%
Array pop (Full) 1431.0±41.96µs 1464.7±57.17µs -2.30%
Boolean Object Access (Execution) 5.7±0.29µs 6.0±0.38µs -5.00%
Boolean Object Access (Full) 292.4±7.80µs 289.0±9.76µs +1.18%
Clean js (Execution) 732.5±50.36µs 728.9±30.78µs +0.49%
Clean js (Full) 1082.6±83.40µs 1022.4±40.53µs +5.89%
Clean js (Parser) 44.1±1.55µs 44.2±1.45µs -0.23%
Create Realm 459.2±14.74ns 489.6±19.13ns -6.21%
Dynamic Object Property Access (Execution) 5.8±0.16µs 5.9±0.28µs -1.69%
Dynamic Object Property Access (Full) 298.9±12.56µs 283.3±9.64µs +5.51%
Expression (Parser) 7.6±0.47µs 7.3±0.52µs +4.11%
Fibonacci (Execution) 986.1±37.83µs 1015.0±47.73µs -2.85%
Fibonacci (Full) 1293.9±70.60µs 1288.2±50.55µs +0.44%
For loop (Execution) 25.1±1.28µs 26.4±1.49µs -4.92%
For loop (Full) 304.2±11.92µs 317.7±12.13µs -4.25%
For loop (Parser) 21.0±0.90µs 21.1±1.00µs -0.47%
Goal Symbols (Parser) 14.9±1.20µs 14.8±0.69µs +0.68%
Hello World (Parser) 3.9±0.21µs 3.8±0.20µs +2.63%
Long file (Parser) 812.9±37.11ns 816.0±41.22ns -0.38%
Mini js (Execution) 648.4±18.18µs 654.8±50.85µs -0.98%
Mini js (Full) 989.8±49.74µs 944.2±34.17µs +4.83%
Mini js (Parser) 39.5±1.42µs 38.7±1.84µs +2.07%
Number Object Access (Execution) 4.4±0.23µs 4.6±0.24µs -4.35%
Number Object Access (Full) 289.8±13.70µs 278.4±11.26µs +4.09%
Object Creation (Execution) 4.9±0.20µs 5.2±0.27µs -5.77%
Object Creation (Full) 283.1±7.10µs 285.5±16.52µs -0.84%
RegExp (Execution) 10.5±0.26µs 10.4±0.70µs +0.96%
RegExp (Full) 302.8±9.52µs 298.4±14.95µs +1.47%
RegExp Literal (Execution) 11.9±0.60µs 11.9±0.59µs 0.00%
RegExp Literal (Full) 300.4±8.26µs 302.1±10.93µs -0.56%
RegExp Literal Creation (Execution) 10.4±0.66µs 10.3±0.52µs +0.97%
RegExp Literal Creation (Full) 297.5±14.81µs 290.1±11.14µs +2.55%
Static Object Property Access (Execution) 5.2±0.20µs 5.3±0.22µs -1.89%
Static Object Property Access (Full) 291.8±13.14µs 283.9±14.26µs +2.78%
String Object Access (Execution) 7.9±0.31µs 8.1±0.31µs -2.47%
String Object Access (Full) 289.3±7.40µs 288.3±26.90µs +0.35%
String comparison (Execution) 7.2±0.37µs 7.1±0.28µs +1.41%
String comparison (Full) 290.2±20.50µs 283.6±9.70µs +2.33%
String concatenation (Execution) 5.9±0.28µs 5.9±0.23µs 0.00%
String concatenation (Full) 287.4±9.33µs 292.2±20.23µs -1.64%
String copy (Execution) 4.7±0.17µs 4.6±0.18µs +2.17%
String copy (Full) 276.2±9.58µs 277.7±10.69µs -0.54%
Symbols (Execution) 4.0±0.13µs 4.0±0.15µs 0.00%
Symbols (Full) 266.9±10.68µs 271.8±10.97µs -1.80%

@RageKnify
Copy link
Contributor Author

I think the clippy lint is a false positive, I tried changing the code to respect it but it results in a rustc error.
Might be rust-lang/rust-clippy#6312

@RageKnify RageKnify added this to the v0.12.0 milestone Jan 13, 2021
@Razican Razican added the enhancement New feature or request label Jan 13, 2021
Copy link
Contributor

@tofpie tofpie 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 👍


// Allow identification of the global object easily
global.set_data(crate::object::ObjectData::Global);
global.data = ObjectData::Global;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add attribute #[allow(clippy::field_reassign_with_default)] on the create method to make clippy happy.

Copy link
Member

Choose a reason for hiding this comment

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

Does clippy offer a better suggestion?

Copy link
Contributor Author

@RageKnify RageKnify Jan 14, 2021

Choose a reason for hiding this comment

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

The code was triggering a clippy lint, but it seems to be a false positive since the code proposed by clippy wasn't valid.

@github-actions
Copy link

Benchmark for 030a58c

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 300.3±0.17ns 266.0±0.15ns +12.89%
Arithmetic operations (Full) 173.6±0.75µs 170.9±0.19µs +1.58%
Array access (Execution) 4.6±0.02µs 4.7±0.02µs -2.13%
Array access (Full) 191.4±6.27µs 187.6±0.53µs +2.03%
Array creation (Execution) 2.1±0.00ms 2.3±0.01ms -8.70%
Array creation (Full) 2.7±0.00ms 2.4±0.00ms +12.50%
Array pop (Execution) 687.3±1.45µs 727.6±2.49µs -5.54%
Array pop (Full) 1049.7±2.40µs 1044.2±5.13µs +0.53%
Boolean Object Access (Execution) 3.9±0.00µs 4.5±0.01µs -13.33%
Boolean Object Access (Full) 188.2±0.55µs 186.7±0.28µs +0.80%
Clean js (Execution) 567.1±2.91µs 576.9±3.53µs -1.70%
Clean js (Full) 766.7±44.76µs 724.4±3.98µs +5.84%
Clean js (Parser) 31.2±0.05µs 35.2±0.15µs -11.36%
Create Realm 379.9±0.94ns 419.3±5.00ns -9.40%
Dynamic Object Property Access (Execution) 4.3±0.02µs 3.9±0.01µs +10.26%
Dynamic Object Property Access (Full) 212.5±0.71µs 184.5±0.18µs +15.18%
Expression (Parser) 5.2±0.00µs 5.9±0.01µs -11.86%
Fibonacci (Execution) 636.8±1.07µs 634.1±0.77µs +0.43%
Fibonacci (Full) 843.6±0.80µs 842.8±1.19µs +0.09%
For loop (Execution) 17.1±0.05µs 17.1±0.05µs 0.00%
For loop (Full) 205.0±0.42µs 202.4±0.21µs +1.28%
For loop (Parser) 15.0±0.04µs 16.9±0.03µs -11.24%
Goal Symbols (Parser) 10.4±0.02µs 11.8±0.01µs -11.86%
Hello World (Parser) 2.8±0.01µs 3.2±0.01µs -12.50%
Long file (Parser) 644.4±4.79ns 696.1±0.46ns -7.43%
Mini js (Execution) 509.4±3.00µs 519.9±2.97µs -2.02%
Mini js (Full) 667.9±4.38µs 653.1±2.27µs +2.27%
Mini js (Parser) 27.5±0.26µs 27.5±0.04µs 0.00%
Number Object Access (Execution) 3.1±0.00µs 3.6±0.00µs -13.89%
Number Object Access (Full) 184.6±0.49µs 181.8±0.42µs +1.54%
Object Creation (Execution) 3.2±0.01µs 3.3±0.01µs -3.03%
Object Creation (Full) 184.6±0.33µs 181.6±0.23µs +1.65%
RegExp (Execution) 8.2±0.03µs 7.1±0.01µs +15.49%
RegExp (Full) 193.6±0.29µs 191.2±0.56µs +1.26%
RegExp Literal (Execution) 9.3±0.03µs 8.2±0.04µs +13.41%
RegExp Literal (Full) 193.1±0.33µs 188.8±0.24µs +2.28%
RegExp Literal Creation (Execution) 8.2±0.04µs 7.1±0.01µs +15.49%
RegExp Literal Creation (Full) 189.0±0.49µs 184.2±0.25µs +2.61%
Static Object Property Access (Execution) 3.5±0.01µs 3.5±0.01µs 0.00%
Static Object Property Access (Full) 185.8±0.48µs 183.0±0.46µs +1.53%
String Object Access (Execution) 6.0±0.01µs 6.1±0.18µs -1.64%
String Object Access (Full) 215.0±1.14µs 186.1±0.45µs +15.53%
String comparison (Execution) 4.8±0.02µs 4.9±0.11µs -2.04%
String comparison (Full) 187.4±0.23µs 186.9±0.40µs +0.27%
String concatenation (Execution) 3.9±0.02µs 4.0±0.02µs -2.50%
String concatenation (Full) 181.8±0.37µs 181.8±0.20µs 0.00%
String copy (Execution) 2.9±0.03µs 3.4±0.01µs -14.71%
String copy (Full) 178.6±0.90µs 177.9±0.28µs +0.39%
Symbols (Execution) 2.5±0.01µs 2.5±0.01µs 0.00%
Symbols (Full) 199.1±0.29µs 197.0±0.27µs +1.07%

@jasonwilliams jasonwilliams deleted the refactor/global_gcobject branch January 16, 2021 14:31
@jasonwilliams
Copy link
Member

Sorry i think i removed this branch accidently, i was doing some cleaning up. If you have a locally copy could you pus again?

@RageKnify RageKnify restored the refactor/global_gcobject branch January 16, 2021 14:45
@RageKnify RageKnify reopened this Jan 16, 2021
@github-actions
Copy link

Benchmark for 06c5c2a

Click to view benchmark
Test PR Benchmark Master Benchmark %
Arithmetic operations (Execution) 360.2±0.38ns 365.6±0.32ns -1.48%
Arithmetic operations (Full) 238.1±0.36µs 236.8±0.54µs +0.55%
Array access (Execution) 6.2±0.03µs 6.3±0.02µs -1.59%
Array access (Full) 260.2±0.48µs 257.7±0.62µs +0.97%
Array creation (Execution) 3.1±0.01ms 2.9±0.00ms +6.90%
Array creation (Full) 3.4±0.02ms 3.4±0.00ms 0.00%
Array pop (Execution) 990.0±3.19µs 943.0±2.84µs +4.98%
Array pop (Full) 1452.3±16.48µs 1454.5±2.35µs -0.15%
Boolean Object Access (Execution) 5.2±0.01µs 5.2±0.01µs 0.00%
Boolean Object Access (Full) 256.0±0.37µs 255.8±0.83µs +0.08%
Clean js (Execution) 688.2±3.69µs 679.7±2.89µs +1.25%
Clean js (Full) 999.4±12.23µs 993.8±6.77µs +0.56%
Clean js (Parser) 40.4±0.07µs 40.6±0.06µs -0.49%
Create Realm 466.9±1.61ns 494.7±0.63ns -5.62%
Dynamic Object Property Access (Execution) 5.1±0.01µs 5.0±0.01µs +2.00%
Dynamic Object Property Access (Full) 257.3±1.55µs 253.7±0.38µs +1.42%
Expression (Parser) 7.1±0.01µs 7.1±0.01µs 0.00%
Fibonacci (Execution) 851.0±0.71µs 853.7±1.43µs -0.32%
Fibonacci (Full) 1133.8±2.24µs 1155.9±1.98µs -1.91%
For loop (Execution) 23.1±0.14µs 23.1±0.08µs 0.00%
For loop (Full) 280.9±0.43µs 278.4±2.09µs +0.90%
For loop (Parser) 19.6±0.03µs 19.6±0.01µs 0.00%
Goal Symbols (Parser) 13.7±0.02µs 13.8±0.02µs -0.72%
Hello World (Parser) 3.7±0.01µs 3.7±0.01µs 0.00%
Long file (Parser) 794.0±0.66ns 805.6±1.00ns -1.44%
Mini js (Execution) 614.1±3.86µs 602.9±3.43µs +1.86%
Mini js (Full) 901.6±6.58µs 898.4±3.11µs +0.36%
Mini js (Parser) 35.8±0.06µs 36.0±0.05µs -0.56%
Number Object Access (Execution) 4.1±0.01µs 4.0±0.01µs +2.50%
Number Object Access (Full) 250.8±0.38µs 251.8±0.53µs -0.40%
Object Creation (Execution) 4.3±0.02µs 4.3±0.01µs 0.00%
Object Creation (Full) 251.6±0.54µs 248.8±0.47µs +1.13%
RegExp (Execution) 9.6±0.03µs 9.7±0.05µs -1.03%
RegExp (Full) 267.5±1.88µs 262.9±0.47µs +1.75%
RegExp Literal (Execution) 11.0±0.03µs 11.1±0.03µs -0.90%
RegExp Literal (Full) 262.8±0.75µs 260.5±0.60µs +0.88%
RegExp Literal Creation (Execution) 9.6±0.03µs 9.7±0.03µs -1.03%
RegExp Literal Creation (Full) 256.7±0.55µs 253.3±0.47µs +1.34%
Static Object Property Access (Execution) 4.6±0.01µs 4.6±0.01µs 0.00%
Static Object Property Access (Full) 252.5±0.88µs 250.9±0.43µs +0.64%
String Object Access (Execution) 7.1±0.02µs 7.1±0.02µs 0.00%
String Object Access (Full) 257.7±0.58µs 257.0±0.29µs +0.27%
String comparison (Execution) 6.6±0.02µs 6.4±0.01µs +3.12%
String comparison (Full) 257.2±0.63µs 257.9±0.81µs -0.27%
String concatenation (Execution) 5.3±0.02µs 5.4±0.02µs -1.85%
String concatenation (Full) 250.5±0.59µs 248.6±0.45µs +0.76%
String copy (Execution) 3.9±0.02µs 4.0±0.05µs -2.50%
String copy (Full) 245.1±0.48µs 243.7±0.44µs +0.57%
Symbols (Execution) 3.4±0.01µs 3.3±0.04µs +3.03%
Symbols (Full) 240.5±0.31µs 239.3±0.35µs +0.50%

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 to go from my side.

@RageKnify RageKnify merged commit 5a5061c into master Jan 17, 2021
@RageKnify RageKnify deleted the refactor/global_gcobject branch January 17, 2021 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants