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

Fixing global object not being populated with the global object properties #2251

Closed
wants to merge 1 commit into from

Conversation

Razican
Copy link
Member

@Razican Razican commented Aug 22, 2022

This Pull Request fixes part of #1987.

It changes the following:

  • When a built-in gets initialized, not only the global binding gets updated, but the global object receives that built-in as a property.

This is a fairly inefficient way of doing this, since it clones the property and assigns it to the global object. Ideally, if I'm not mistaken, these properties shouldn't be declared in the global bindings, they should just be properties of the global binding, so we should avoid creating the global binding. Nevertheless, this, as I mentioned in #1987, this breaks a ton of tests.

The issue here is that the naming resolution at global level doesn't check the global object, unfortunately. Should this be changed?

Also, this doesn't solve the full issue, since the main problem of #1987 comes from the fact that the global object doesn't have any prototype, but as per the spec:

has a [[Prototype]] internal slot whose value is host-defined.

Full spec

I could not find anywhere that the prototype should be the Object prototype, though. This would require some further changes to be fixed.

@Razican Razican added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics run-benchmark Label used to run banchmarks on PRs labels Aug 22, 2022
@Razican Razican added this to the v0.16.0 milestone Aug 22, 2022
@github-actions
Copy link

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 91,733 91,733 0
Passed 64,908 64,908 0
Ignored 14,750 14,750 0
Failed 12,075 12,075 0
Panics 0 0 0
Conformance 70.76% 70.76% 0.00%

@codecov
Copy link

codecov bot commented Aug 22, 2022

Codecov Report

Merging #2251 (790a93c) into main (f0caa3c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2251      +/-   ##
==========================================
+ Coverage   41.31%   41.33%   +0.02%     
==========================================
  Files         234      234              
  Lines       22016    22017       +1     
==========================================
+ Hits         9095     9101       +6     
+ Misses      12921    12916       -5     
Impacted Files Coverage Δ
boa_engine/src/builtins/mod.rs 17.14% <100.00%> (+5.20%) ⬆️
boa_engine/src/object/mod.rs 20.57% <0.00%> (-0.08%) ⬇️
...a_engine/src/syntax/ast/node/statement_list/mod.rs 81.25% <0.00%> (+3.12%) ⬆️
boa_engine/src/builtins/set/ordered_set.rs 14.81% <0.00%> (+7.40%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link

Benchmark for 3f2fb73

Click to view benchmark
Test Base PR %
Arithmetic operations (Compiler) 549.4±0.92ns 532.2±2.52ns -3.13%
Arithmetic operations (Execution) 670.7±0.89ns 686.1±7.14ns +2.30%
Arithmetic operations (Parser) 6.5±0.01µs 6.5±0.02µs 0.00%
Array access (Compiler) 1427.8±2.39ns 1384.4±2.53ns -3.04%
Array access (Execution) 7.2±0.02µs 7.2±0.02µs 0.00%
Array access (Parser) 13.6±0.02µs 13.5±0.02µs -0.74%
Array creation (Compiler) 2.2±0.01µs 2.2±0.01µs 0.00%
Array creation (Execution) 1096.4±2.72µs 1083.8±8.33µs -1.15%
Array creation (Parser) 15.8±0.02µs 16.0±0.04µs +1.27%
Array pop (Compiler) 4.1±0.01µs 4.1±0.01µs 0.00%
Array pop (Execution) 569.4±2.25µs 569.4±6.69µs 0.00%
Array pop (Parser) 147.2±0.21µs 147.5±0.14µs +0.20%
Boolean Object Access (Compiler) 1142.7±8.79ns 1138.6±3.78ns -0.36%
Boolean Object Access (Execution) 4.2±0.01µs 4.3±0.01µs +2.38%
Boolean Object Access (Parser) 16.1±0.03µs 16.0±0.03µs -0.62%
Clean js (Compiler) 4.7±0.01µs 4.6±0.01µs -2.13%
Clean js (Execution) 605.5±3.01µs 594.3±3.25µs -1.85%
Clean js (Parser) 32.4±0.04µs 32.5±0.21µs +0.31%
Create Realm 236.6±0.61ns 228.7±1.37ns -3.34%
Dynamic Object Property Access (Compiler) 1705.5±2.51ns 1666.1±3.18ns -2.31%
Dynamic Object Property Access (Execution) 4.7±0.02µs 4.7±0.03µs 0.00%
Dynamic Object Property Access (Parser) 12.5±0.01µs 12.4±0.02µs -0.80%
Fibonacci (Compiler) 2.6±0.01µs 2.6±0.00µs 0.00%
Fibonacci (Execution) 1028.3±2.27µs 1012.7±7.42µs -1.52%
Fibonacci (Parser) 18.8±0.02µs 18.6±0.13µs -1.06%
For loop (Compiler) 2.5±0.01µs 2.5±0.01µs 0.00%
For loop (Execution) 16.2±0.04µs 16.4±0.03µs +1.23%
For loop (Parser) 16.2±0.03µs 16.2±0.05µs 0.00%
Mini js (Compiler) 4.1±0.01µs 4.0±0.01µs -2.44%
Mini js (Execution) 552.9±18.40µs 544.6±5.53µs -1.50%
Mini js (Parser) 28.4±0.04µs 28.5±0.10µs +0.35%
Number Object Access (Compiler) 1069.8±2.68ns 1060.9±1.97ns -0.83%
Number Object Access (Execution) 3.3±0.01µs 3.3±0.01µs 0.00%
Number Object Access (Parser) 12.4±0.02µs 12.3±0.01µs -0.81%
Object Creation (Compiler) 1485.2±7.03ns 1454.2±3.53ns -2.09%
Object Creation (Execution) 4.5±0.02µs 4.4±0.02µs -2.22%
Object Creation (Parser) 10.9±0.02µs 10.8±0.01µs -0.92%
RegExp (Compiler) 1680.7±3.52ns 1672.1±4.02ns -0.51%
RegExp (Execution) 10.9±0.04µs 11.4±0.03µs +4.59%
RegExp (Parser) 11.9±0.02µs 11.8±0.02µs -0.84%
RegExp Creation (Compiler) 1511.1±6.36ns 1489.3±8.59ns -1.44%
RegExp Creation (Execution) 8.1±0.02µs 8.4±0.02µs +3.70%
RegExp Creation (Parser) 10.0±0.01µs 9.8±0.01µs -2.00%
RegExp Literal (Compiler) 1682.4±3.01ns 1654.0±4.19ns -1.69%
RegExp Literal (Execution) 10.9±0.03µs 11.4±0.02µs +4.59%
RegExp Literal (Parser) 9.7±0.01µs 9.6±0.01µs -1.03%
RegExp Literal Creation (Compiler) 1505.7±6.62ns 1477.4±6.29ns -1.88%
RegExp Literal Creation (Execution) 8.1±0.03µs 8.4±0.02µs +3.70%
RegExp Literal Creation (Parser) 7.6±0.02µs 7.5±0.01µs -1.32%
Static Object Property Access (Compiler) 1519.7±6.66ns 1493.6±5.78ns -1.72%
Static Object Property Access (Execution) 4.6±0.03µs 4.6±0.02µs 0.00%
Static Object Property Access (Parser) 11.7±0.02µs 11.5±0.01µs -1.71%
String Object Access (Compiler) 1422.7±5.96ns 1407.4±10.18ns -1.08%
String Object Access (Execution) 6.0±0.02µs 6.0±0.02µs 0.00%
String Object Access (Parser) 15.6±0.02µs 15.4±0.01µs -1.28%
String comparison (Compiler) 2.2±0.01µs 2.1±0.00µs -4.55%
String comparison (Execution) 4.0±0.01µs 4.0±0.01µs 0.00%
String comparison (Parser) 12.6±0.02µs 12.5±0.01µs -0.79%
String concatenation (Compiler) 1688.8±2.67ns 1632.5±3.79ns -3.33%
String concatenation (Execution) 3.8±0.01µs 3.7±0.04µs -2.63%
String concatenation (Parser) 8.9±0.01µs 8.8±0.02µs -1.12%
String copy (Compiler) 1409.0±3.21ns 1352.7±3.91ns -4.00%
String copy (Execution) 3.5±0.01µs 3.5±0.01µs 0.00%
String copy (Parser) 6.7±0.01µs 6.6±0.02µs -1.49%
Symbols (Compiler) 1047.2±1.78ns 1036.6±7.80ns -1.01%
Symbols (Execution) 3.5±0.01µs 3.5±0.01µs 0.00%
Symbols (Parser) 5.3±0.01µs 5.1±0.01µs -3.77%

@raskad
Copy link
Member

raskad commented Aug 23, 2022

I this we should solve this is a different way. I put a full explanation in the linked issue: #1987 (comment)

@Razican
Copy link
Member Author

Razican commented Aug 23, 2022

I agree we should solve this properly :) closing this PR in favor of that approach, feel free to open a PR about it.

@Razican Razican closed this Aug 23, 2022
@jedel1043 jedel1043 deleted the global_obj_fix branch April 19, 2023 01:40
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 run-benchmark Label used to run banchmarks on PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants