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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Fix PropertyKey to JsValue conversion #1886

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Fix PropertyKey to JsValue conversion #1886

wants to merge 1 commit into from

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented Mar 2, 2022 •
edited
Loading

We store string PropertyKeys with two enums String and Index for performance reasons, but the spec does not differentiate between string and index property keys so before conversion to JsValue we have to convert to a string.

This was failing tests like Reflect.ownKeys([true, "", 1]) because it was returning (integer numbers) [1, 2, 3] instead of ['1', '2', '3']

@HalidOdat HalidOdat added bug Something isn't working execution Issues or PRs related to code execution labels Mar 2, 2022
@HalidOdat HalidOdat added this to the v0.14.0 milestone Mar 2, 2022
@github-actions
Copy link

github-actions bot commented Mar 2, 2022

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 88,342 88,342 0
Passed 43,131 43,195 +64
Ignored 21,413 21,413 0
Failed 23,798 23,734 -64
Panics 0 0 0
Conformance 48.82% 48.90% +0.07%
Fixed tests (64): 
test/language/expressions/object/object-spread-proxy-no-excluded-keys.js [strict mode] (previously Failed)
test/language/expressions/object/object-spread-proxy-no-excluded-keys.js (previously Failed)
test/language/expressions/object/object-spread-proxy-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/language/expressions/object/object-spread-proxy-ownkeys-returned-keys-order.js (previously Failed)
test/language/expressions/object/object-spread-proxy-get-not-called-on-dontenum-keys.js [strict mode] (previously Failed)
test/language/expressions/object/object-spread-proxy-get-not-called-on-dontenum-keys.js (previously Failed)
test/language/expressions/object/dstr/object-rest-proxy-get-not-called-on-dontenum-keys.js [strict mode] (previously Failed)
test/language/expressions/object/dstr/object-rest-proxy-get-not-called-on-dontenum-keys.js (previously Failed)
test/language/expressions/object/dstr/object-rest-proxy-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/language/expressions/object/dstr/object-rest-proxy-ownkeys-returned-keys-order.js (previously Failed)
test/built-ins/Array/prototype/flatMap/proxy-access-count.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/flatMap/proxy-access-count.js (previously Failed)
test/built-ins/Array/prototype/copyWithin/return-abrupt-from-delete-proxy-target.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/copyWithin/return-abrupt-from-delete-proxy-target.js (previously Failed)
test/built-ins/Array/prototype/splice/create-species-length-exceeding-integer-limit.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/splice/create-species-length-exceeding-integer-limit.js (previously Failed)
test/built-ins/Array/prototype/flat/proxy-access-count.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/flat/proxy-access-count.js (previously Failed)
test/built-ins/Array/prototype/includes/get-prop.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/includes/get-prop.js (previously Failed)
test/built-ins/Reflect/ownKeys/return-non-enumerable-keys.js [strict mode] (previously Failed)
test/built-ins/Reflect/ownKeys/return-non-enumerable-keys.js (previously Failed)
test/built-ins/Reflect/ownKeys/return-on-corresponding-order.js [strict mode] (previously Failed)
test/built-ins/Reflect/ownKeys/return-on-corresponding-order.js (previously Failed)
test/built-ins/Object/isFrozen/proxy-no-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/built-ins/Object/isFrozen/proxy-no-ownkeys-returned-keys-order.js (previously Failed)
test/built-ins/Object/isSealed/proxy-no-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/built-ins/Object/isSealed/proxy-no-ownkeys-returned-keys-order.js (previously Failed)
test/built-ins/Object/assign/strings-and-symbol-order-proxy.js [strict mode] (previously Failed)
test/built-ins/Object/assign/strings-and-symbol-order-proxy.js (previously Failed)
test/built-ins/Object/freeze/proxy-no-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/built-ins/Object/freeze/proxy-no-ownkeys-returned-keys-order.js (previously Failed)
test/built-ins/Object/seal/proxy-no-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/built-ins/Object/seal/proxy-no-ownkeys-returned-keys-order.js (previously Failed)
test/built-ins/Object/getOwnPropertyDescriptors/proxy-no-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/built-ins/Object/getOwnPropertyDescriptors/proxy-no-ownkeys-returned-keys-order.js (previously Failed)
test/built-ins/Object/defineProperties/proxy-no-ownkeys-returned-keys-order.js [strict mode] (previously Failed)
test/built-ins/Object/defineProperties/proxy-no-ownkeys-returned-keys-order.js (previously Failed)
test/built-ins/Proxy/has/trap-is-null-target-is-proxy.js [strict mode] (previously Failed)
test/built-ins/Proxy/has/trap-is-null-target-is-proxy.js (previously Failed)
test/built-ins/Proxy/has/call-in-prototype-index.js [strict mode] (previously Failed)
test/built-ins/Proxy/has/call-in-prototype-index.js (previously Failed)
test/built-ins/Proxy/get/trap-is-null-target-is-proxy.js [strict mode] (previously Failed)
test/built-ins/Proxy/get/trap-is-null-target-is-proxy.js (previously Failed)
test/built-ins/Proxy/ownKeys/trap-is-missing-target-is-proxy.js [strict mode] (previously Failed)
test/built-ins/Proxy/ownKeys/trap-is-missing-target-is-proxy.js (previously Failed)
test/built-ins/Proxy/ownKeys/trap-is-undefined-target-is-proxy.js [strict mode] (previously Failed)
test/built-ins/Proxy/ownKeys/trap-is-undefined-target-is-proxy.js (previously Failed)
test/built-ins/Proxy/set/trap-is-missing-receiver-multiple-calls-index.js [strict mode] (previously Failed)
test/built-ins/Proxy/set/trap-is-missing-receiver-multiple-calls-index.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/integer-indexes-and-string-and-symbol-keys-.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/integer-indexes-and-string-and-symbol-keys-.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/integer-indexes.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/integer-indexes.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/integer-indexes-and-string-keys.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/integer-indexes-and-string-keys.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/BigInt/integer-indexes-and-string-and-symbol-keys-.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/BigInt/integer-indexes-and-string-and-symbol-keys-.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/BigInt/integer-indexes.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/BigInt/integer-indexes.js (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/BigInt/integer-indexes-and-string-keys.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/internals/OwnPropertyKeys/BigInt/integer-indexes-and-string-keys.js (previously Failed)
test/intl402/Intl/getCanonicalLocales/has-property.js [strict mode] (previously Failed)
test/intl402/Intl/getCanonicalLocales/has-property.js (previously Failed)

@codecov
Copy link

codecov bot commented Mar 2, 2022 •
edited
Loading

Codecov Report

Merging #1886 (7daecba) into main (7248ed1) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1886   +/-   ##
=======================================
  Coverage   46.75%   46.76%           
=======================================
  Files         204      204           
  Lines       16755    16752    -3     
=======================================
  Hits         7834     7834           
+ Misses       8921     8918    -3
Impacted Files Coverage Δ
boa_engine/src/property/mod.rs 49.36% <0.00%> (+0.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 7248ed1...7daecba. Read the comment docs.

jedel1043
jedel1043 approved these changes Mar 2, 2022
View reviewed changes
Copy link
Member

@jedel1043 jedel1043 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!

@github-actions
Copy link

github-actions bot commented Mar 2, 2022

Benchmark for dc17f79

Click to view benchmark
Test Base PR %
Arithmetic operations (Compiler) 441.8±3.64ns 513.2±1.74ns +16.16%
Arithmetic operations (Execution) 1744.5±3.31ns 2.0±0.00µs +14.65%
Arithmetic operations (Parser) 4.6±0.00µs 4.6±0.01µs 0.00%
Array access (Compiler) 1102.5±2.79ns 1115.7±1.92ns +1.20%
Array access (Execution) 9.7±0.07µs 9.8±0.04µs +1.03%
Array access (Parser) 10.2±0.02µs 11.4±0.01µs +11.76%
Array creation (Compiler) 1608.6±3.90ns 1641.9±6.87ns +2.07%
Array creation (Execution) 3.2±0.01ms 3.1±0.01ms -3.13%
Array creation (Parser) 11.4±0.01µs 11.4±0.03µs 0.00%
Array pop (Compiler) 3.9±0.01µs 3.5±0.01µs -10.26%
Array pop (Execution) 1221.6±3.45µs 1219.0±5.36µs -0.21%
Array pop (Parser) 129.7±0.13µs 129.4±0.10µs -0.23%
Boolean Object Access (Compiler) 1089.2±4.40ns 984.3±47.90ns -9.63%
Boolean Object Access (Execution) 5.1±0.02µs 5.1±0.04µs 0.00%
Boolean Object Access (Parser) 12.4±0.09µs 12.4±0.02µs 0.00%
Clean js (Compiler) 3.0±0.01µs 3.0±0.01µs 0.00%
Clean js (Execution) 969.5±4.39µs 1087.7±7.98µs +12.19%
Clean js (Parser) 24.8±0.05µs 24.6±0.10µs -0.81%
Create Realm 233.1±0.26ns 270.5±0.49ns +16.04%
Dynamic Object Property Access (Compiler) 1427.2±5.17ns 1440.4±6.03ns +0.92%
Dynamic Object Property Access (Execution) 6.7±0.04µs 6.8±0.03µs +1.49%
Dynamic Object Property Access (Parser) 9.0±0.03µs 9.1±0.01µs +1.11%
Fibonacci (Compiler) 1974.1±4.87ns 1982.8±3.25ns +0.44%
Fibonacci (Execution) 1551.9±1.92µs 1763.4±2.71µs +13.63%
Fibonacci (Parser) 13.8±0.03µs 13.7±0.03µs -0.72%
For loop (Compiler) 1950.1±5.58ns 1720.5±5.53ns -11.77%
For loop (Execution) 37.2±0.14µs 36.9±0.12µs -0.81%
For loop (Parser) 11.7±0.03µs 11.7±0.05µs 0.00%
Mini js (Compiler) 2.8±0.01µs 2.9±0.02µs +3.57%
Mini js (Execution) 893.4±4.44µs 997.9±7.64µs +11.70%
Mini js (Parser) 21.6±0.02µs 21.5±0.03µs -0.46%
Number Object Access (Compiler) 895.0±2.67ns 1040.8±4.42ns +16.29%
Number Object Access (Execution) 3.9±0.02µs 4.0±0.02µs +2.56%
Number Object Access (Parser) 10.9±0.01µs 10.9±0.01µs 0.00%
Object Creation (Compiler) 1219.8±3.64ns 1383.7±5.72ns +13.44%
Object Creation (Execution) 5.4±0.12µs 6.1±0.02µs +12.96%
Object Creation (Parser) 7.9±0.02µs 8.9±0.02µs +12.66%
RegExp (Compiler) 1428.8±4.76ns 1448.8±4.98ns +1.40%
RegExp (Execution) 12.6±0.04µs 10.7±0.03µs -15.08%
RegExp (Parser) 8.6±0.02µs 9.9±0.10µs +15.12%
RegExp Creation (Compiler) 1210.8±4.87ns 1269.4±2.55ns +4.84%
RegExp Creation (Execution) 9.4±0.04µs 9.1±0.04µs -3.19%
RegExp Creation (Parser) 8.2±0.02µs 7.3±0.05µs -10.98%
RegExp Literal (Compiler) 1409.2±5.98ns 1451.4±10.40ns +2.99%
RegExp Literal (Execution) 12.5±0.04µs 12.2±0.03µs -2.40%
RegExp Literal (Parser) 7.9±0.05µs 8.0±0.02µs +1.27%
RegExp Literal Creation (Compiler) 1214.4±4.05ns 1265.8±2.98ns +4.23%
RegExp Literal Creation (Execution) 9.3±0.05µs 9.0±0.04µs -3.23%
RegExp Literal Creation (Parser) 6.3±0.01µs 5.5±0.01µs -12.70%
Static Object Property Access (Compiler) 1216.4±3.70ns 1234.0±4.27ns +1.45%
Static Object Property Access (Execution) 6.4±0.02µs 6.4±0.02µs 0.00%
Static Object Property Access (Parser) 9.6±0.02µs 8.5±0.02µs -11.46%
String Object Access (Compiler) 1279.2±4.83ns 1302.6±9.21ns +1.83%
String Object Access (Execution) 6.8±0.02µs 6.8±0.05µs 0.00%
String Object Access (Parser) 13.8±0.01µs 12.2±0.03µs -11.59%
String comparison (Compiler) 1844.5±10.01ns 2.1±0.01µs +13.85%
String comparison (Execution) 5.2±0.02µs 5.2±0.02µs 0.00%
String comparison (Parser) 10.6±0.02µs 9.4±0.01µs -11.32%
String concatenation (Compiler) 1419.0±5.87ns 1434.7±3.86ns +1.11%
String concatenation (Execution) 4.7±0.02µs 4.7±0.02µs 0.00%
String concatenation (Parser) 7.3±0.02µs 6.5±0.01µs -10.96%
String copy (Compiler) 1123.3±2.76ns 1300.6±8.48ns +15.78%
String copy (Execution) 4.2±0.01µs 4.2±0.01µs 0.00%
String copy (Parser) 5.5±0.02µs 4.8±0.01µs -12.73%
Symbols (Compiler) 792.1±1.72ns 824.1±1.64ns +4.04%
Symbols (Execution) 3.9±0.01µs 4.0±0.01µs +2.56%
Symbols (Parser) 3.8±0.01µs 4.3±0.01µs +13.16%

@HalidOdat
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Mar 2, 2022
@HalidOdat
Fix PropertyKey to JsValue conversion (#1886)
fd889fd 
We store string `PropertyKey`s with two enums `String` and `Index` for performance reasons, but the spec does not differentiate between string and index property keys so before conversion to `JsValue` we have to convert to a string.

This was failing tests like `Reflect.ownKeys([true, "", 1])` because it was returning (integer numbers) `[1, 2, 3]` instead of `['1', '2', '3']`
@bors
Copy link

bors bot commented Mar 2, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix PropertyKey to JsValue conversion [Merged by Bors] - Fix PropertyKey to JsValue conversion Mar 2, 2022
@bors bors bot closed this Mar 2, 2022
@bors bors bot deleted the fix/PropertyKey-to-JsValue branch March 2, 2022 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedel1043 jedel1043 jedel1043 approved these changes

@raskad raskad raskad approved these changes

@Razican Razican Awaiting requested review from Razican

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

@RageKnify RageKnify Awaiting requested review from RageKnify

Assignees
No one assigned
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Milestone
v0.14.0
Development

Successfully merging this pull request may close these issues.
3 participants
@HalidOdat @raskad @jedel1043