Changeset 267040 in webkit


Ignore:
Timestamp:
Sep 14, 2020 2:30:25 PM (4 years ago)
Author:
Alexey Shvayka
Message:

Proxy's "ownKeys" trap result should not be sorted
https://bugs.webkit.org/show_bug.cgi?id=216227

Reviewed by Yusuke Suzuki.

JSTests:

  • microbenchmarks/object-get-own-property-symbols-on-large-array.js:
  • microbenchmarks/object-get-own-property-symbols.js:
  • microbenchmarks/reflect-own-keys.js: Added.
  • stress/proxy-own-keys.js: Fix incorrect assert.
  • test262/expectations.yaml: Mark 20 test cases as passing.

Source/JavaScriptCore:

Given that we can't know whether ownPropertyKeys() received property names from
userland Proxy's "ownKeys" trap, this patch moves symbols after strings sorting [1]
to Structure::getPropertyNamesFromStructure(), aligning observed property order
(via Proxy's "getOwnPropertyDescriptor" trap) with V8 and SpiderMonkey.

Also, removes sorting logic duplication in objectConstructorAssign().

This change is neutral on provided Reflect.ownKeys microbenchmark. Although property
name collection besides PropertyNameMode::StringsAndSymbols cases is unaffected,
Object.{keys,getOwnPropertySymbols} microbenchmarks regress by 6-12% due to
increased Structure::getPropertyNamesFromStructure() code size.

[1]: https://tc39.es/ecma262/#sec-ordinaryownpropertykeys (steps 3-4)

  • runtime/ObjectConstructor.cpp:

(JSC::objectConstructorAssign):
(JSC::ownPropertyKeys):

  • runtime/Structure.cpp:

(JSC::Structure::getPropertyNamesFromStructure):

Location:
trunk
Files:
1 added
8 edited

Legend:

Unmodified
Added
Removed
  • trunk/JSTests/ChangeLog

    r267037 r267040  
     12020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
     2
     3        Proxy's "ownKeys" trap result should not be sorted
     4        https://bugs.webkit.org/show_bug.cgi?id=216227
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        * microbenchmarks/object-get-own-property-symbols-on-large-array.js:
     9        * microbenchmarks/object-get-own-property-symbols.js:
     10        * microbenchmarks/reflect-own-keys.js: Added.
     11        * stress/proxy-own-keys.js: Fix incorrect assert.
     12        * test262/expectations.yaml: Mark 20 test cases as passing.
     13
    1142020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
    215
  • trunk/JSTests/microbenchmarks/object-get-own-property-symbols-on-large-array.js

    r251463 r267040  
    44    var buffer = new ArrayBuffer(10000000);
    55    var int8View = new Int8Array(buffer);
    6     var start = Date.now();
    7     var result = Object.getOwnPropertySymbols(int8View);
    8     var delta = Date.now() - start;
    9     if (delta > 1000)
    10         throw new Error(`time consuming (${delta}ms)`);
    11     return result;
     6    for (var i = 0; i < 300000; ++i) {
     7        var start = Date.now();
     8        var result = Object.getOwnPropertySymbols(int8View);
     9        var delta = Date.now() - start;
     10        if (delta > 1000)
     11            throw new Error(`time consuming (${delta}ms)`);
     12    }
    1213}
    1314
  • trunk/JSTests/microbenchmarks/object-get-own-property-symbols.js

    r251463 r267040  
    1111noInline(test);
    1212
    13 for (var i = 0; i < 1e3; ++i)
     13for (var i = 0; i < 2500; ++i)
    1414    test(object);
  • trunk/JSTests/stress/proxy-own-keys.js

    r244330 r267040  
    483483    for (let i = 0; i < 500; i++) {
    484484        let result = Reflect.ownKeys(proxy);
    485         assert(shallowEq(result, ["a", "b", "c", s1, s2]));
     485        assert(shallowEq(result, arr));
    486486        assert(called);
    487487        called = false;
  • trunk/JSTests/test262/expectations.yaml

    r267037 r267040  
    787787  default: 'Test262Error: Conforms to NativeFunction Syntax: "function a(\\u{62}, \\u0063) { \\u0062 = \\u{00063}; return b; }" (function \u0061(\u{62}, \u0063) { \u0062 = \u{00063}; return b; })'
    788788  strict mode: 'Test262Error: Conforms to NativeFunction Syntax: "function a(\\u{62}, \\u0063) { \\u0062 = \\u{00063}; return b; }" (function \u0061(\u{62}, \u0063) { \u0062 = \u{00063}; return b; })'
    789 test/built-ins/Object/assign/strings-and-symbol-order-proxy.js:
    790   default: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
    791   strict mode: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
    792 test/built-ins/Object/defineProperties/proxy-no-ownkeys-returned-keys-order.js:
    793   default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    794   strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    795789test/built-ins/Object/entries/order-after-define-property.js:
    796790  default: 'Test262Error: Expected [a, name] and [name, a] to have the same contents. '
    797791  strict mode: 'Test262Error: Expected [a, name] and [name, a] to have the same contents. '
    798 test/built-ins/Object/freeze/proxy-no-ownkeys-returned-keys-order.js:
    799   default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    800   strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    801 test/built-ins/Object/getOwnPropertyDescriptors/proxy-no-ownkeys-returned-keys-order.js:
    802   default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    803   strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    804792test/built-ins/Object/internals/DefineOwnProperty/consistent-value-function-arguments.js:
    805793  default: 'Test262Error: Expected SameValue(«null», «[object Arguments]») to be true'
     
    809797  default: 'Test262Error: Expected SameValue(«», «x») to be true'
    810798  strict mode: 'Test262Error: Expected SameValue(«», «x») to be true'
    811 test/built-ins/Object/isFrozen/proxy-no-ownkeys-returned-keys-order.js:
    812   default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    813   strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    814 test/built-ins/Object/isSealed/proxy-no-ownkeys-returned-keys-order.js:
    815   default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    816   strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    817799test/built-ins/Object/keys/order-after-define-property.js:
    818800  default: 'Test262Error: Expected [a, length] and [length, a] to have the same contents. '
    819801  strict mode: 'Test262Error: Expected [a, length] and [length, a] to have the same contents. '
    820 test/built-ins/Object/seal/proxy-no-ownkeys-returned-keys-order.js:
    821   default: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    822   strict mode: 'Test262Error: Expected [0, Symbol(), foo] and [0, foo, Symbol()] to have the same contents. '
    823802test/built-ins/Proxy/apply/arguments-realm.js:
    824803  default: 'Test262Error: Expected SameValue(«function Array() {'
     
    857836  default: 'Test262Error: Expected a TypeError but got a TypeError'
    858837  strict mode: 'Test262Error: Expected a TypeError but got a TypeError'
    859 test/built-ins/Proxy/ownKeys/trap-is-undefined-target-is-proxy.js:
    860   default: 'Test262Error: Expected [length, foo, 0, Symbol()] and [Symbol(), length, foo, 0] to have the same contents. '
    861   strict mode: 'Test262Error: Expected [length, foo, 0, Symbol()] and [Symbol(), length, foo, 0] to have the same contents. '
    862838test/built-ins/RegExp/property-escapes/generated/Alphabetic.js:
    863839  default: 'Test262Error: `\p{Alphabetic}` should match U+001CFA (`ᳺ`)'
     
    26602636  default: "SyntaxError: Unexpected escaped characters in keyword token: 'w\\u0069th'"
    26612637  strict mode: "SyntaxError: Unexpected escaped characters in keyword token: 'w\\u0069th'"
    2662 test/language/expressions/object/dstr/object-rest-proxy-ownkeys-returned-keys-order.js:
    2663   default: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
    2664   strict mode: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
    26652638test/language/expressions/object/ident-name-method-def-break-escaped.js:
    26662639  default: "SyntaxError: Unexpected escaped characters in keyword token: 'bre\\u0061k'"
     
    29192892test/language/expressions/object/method-definition/meth-eval-var-scope-syntax-err.js:
    29202893  default: 'Test262Error: Expected a SyntaxError to be thrown but no exception was thrown at all'
    2921 test/language/expressions/object/object-spread-proxy-ownkeys-returned-keys-order.js:
    2922   default: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
    2923   strict mode: 'Test262Error: Expected [foo, 0, Symbol()] and [Symbol(), foo, 0] to have the same contents. '
    29242894test/language/expressions/object/scope-gen-meth-body-lex-distinct.js:
    29252895  default: 'Test262Error: Expected a SyntaxError to be thrown but no exception was thrown at all'
  • trunk/Source/JavaScriptCore/ChangeLog

    r267037 r267040  
     12020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
     2
     3        Proxy's "ownKeys" trap result should not be sorted
     4        https://bugs.webkit.org/show_bug.cgi?id=216227
     5
     6        Reviewed by Yusuke Suzuki.
     7
     8        Given that we can't know whether ownPropertyKeys() received property names from
     9        userland Proxy's "ownKeys" trap, this patch moves symbols after strings sorting [1]
     10        to Structure::getPropertyNamesFromStructure(), aligning observed property order
     11        (via Proxy's "getOwnPropertyDescriptor" trap) with V8 and SpiderMonkey.
     12
     13        Also, removes sorting logic duplication in objectConstructorAssign().
     14
     15        This change is neutral on provided Reflect.ownKeys microbenchmark. Although property
     16        name collection besides PropertyNameMode::StringsAndSymbols cases is unaffected,
     17        Object.{keys,getOwnPropertySymbols} microbenchmarks regress by 6-12% due to
     18        increased Structure::getPropertyNamesFromStructure() code size.
     19
     20        [1]: https://tc39.es/ecma262/#sec-ordinaryownpropertykeys (steps 3-4)
     21
     22        * runtime/ObjectConstructor.cpp:
     23        (JSC::objectConstructorAssign):
     24        (JSC::ownPropertyKeys):
     25        * runtime/Structure.cpp:
     26        (JSC::Structure::getPropertyNamesFromStructure):
     27
    1282020-09-14  Alexey Shvayka  <shvaikalesh@gmail.com>
    229
  • trunk/Source/JavaScriptCore/runtime/ObjectConstructor.cpp

    r265934 r267040  
    364364        RETURN_IF_EXCEPTION(scope, { });
    365365
    366         auto assign = [&] (PropertyName propertyName) {
     366        unsigned numProperties = properties.size();
     367        for (unsigned j = 0; j < numProperties; j++) {
     368            const auto& propertyName = properties[j];
     369            ASSERT(!propertyName.isPrivateName());
     370
    367371            PropertySlot slot(source, PropertySlot::InternalMethodType::GetOwnProperty);
    368372            bool hasProperty = source->methodTable(vm)->getOwnPropertySlot(source, globalObject, propertyName, slot);
    369             RETURN_IF_EXCEPTION(scope, void());
     373            RETURN_IF_EXCEPTION(scope, { });
    370374            if (!hasProperty)
    371                 return;
     375                continue;
    372376            if (slot.attributes() & PropertyAttribute::DontEnum)
    373                 return;
     377                continue;
    374378
    375379            JSValue value;
     
    378382            else
    379383                value = source->get(globalObject, propertyName);
    380             RETURN_IF_EXCEPTION(scope, void());
     384            RETURN_IF_EXCEPTION(scope, { });
    381385
    382386            PutPropertySlot putPropertySlot(target, true);
    383387            target->putInline(globalObject, propertyName, value, putPropertySlot);
    384         };
    385 
    386         // First loop is for strings. Second loop is for symbols to keep standardized order requirement in the spec.
    387         // https://tc39.github.io/ecma262/#sec-ordinaryownpropertykeys
    388         bool foundSymbol = false;
    389         unsigned numProperties = properties.size();
    390         for (unsigned j = 0; j < numProperties; j++) {
    391             const auto& propertyName = properties[j];
    392             if (propertyName.isSymbol()) {
    393                 foundSymbol = true;
    394                 continue;
    395             }
    396 
    397             assign(propertyName);
    398388            RETURN_IF_EXCEPTION(scope, { });
    399         }
    400 
    401         if (foundSymbol) {
    402             for (unsigned j = 0; j < numProperties; j++) {
    403                 const auto& propertyName = properties[j];
    404                 if (propertyName.isSymbol()) {
    405                     ASSERT(!propertyName.isPrivateName());
    406                     assign(propertyName);
    407                     RETURN_IF_EXCEPTION(scope, { });
    408                 }
    409             }
    410389        }
    411390    }
     
    983962
    984963    case PropertyNameMode::StringsAndSymbols: {
    985         Vector<Identifier, 16> propertySymbols;
    986964        size_t numProperties = properties.size();
    987965        for (size_t i = 0; i < numProperties; i++) {
     
    989967            if (identifier.isSymbol()) {
    990968                ASSERT(!identifier.isPrivateName());
    991                 propertySymbols.append(identifier);
    992                 continue;
    993             }
    994 
    995             pushDirect(globalObject, keys, jsOwnedString(vm, identifier.string()));
     969                pushDirect(globalObject, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
     970            } else
     971                pushDirect(globalObject, keys, jsOwnedString(vm, identifier.string()));
    996972            RETURN_IF_EXCEPTION(scope, nullptr);
    997973        }
    998 
    999         // To ensure the order defined in the spec (9.1.12), we append symbols at the last elements of keys.
    1000         for (const auto& identifier : propertySymbols) {
    1001             pushDirect(globalObject, keys, Symbol::create(vm, static_cast<SymbolImpl&>(*identifier.impl())));
    1002             RETURN_IF_EXCEPTION(scope, nullptr);
    1003         }
    1004 
    1005974        break;
    1006975    }
  • trunk/Source/JavaScriptCore/runtime/Structure.cpp

    r266969 r267040  
    12111211   
    12121212    bool knownUnique = propertyNames.canAddKnownUniqueForStructure();
     1213    bool foundSymbol = false;
     1214
     1215    auto checkDontEnumAndAdd = [&](PropertyTable::iterator iter) {
     1216        if (!(iter->attributes & PropertyAttribute::DontEnum) || mode.includeDontEnumProperties()) {
     1217            if (knownUnique)
     1218                propertyNames.addUnchecked(iter->key);
     1219            else
     1220                propertyNames.add(iter->key);
     1221        }
     1222    };
    12131223   
    12141224    PropertyTable::iterator end = table->end();
     
    12161226        ASSERT(!isQuickPropertyAccessAllowedForEnumeration() || !(iter->attributes & PropertyAttribute::DontEnum));
    12171227        ASSERT(!isQuickPropertyAccessAllowedForEnumeration() || !iter->key->isSymbol());
    1218         if (!(iter->attributes & PropertyAttribute::DontEnum) || mode.includeDontEnumProperties()) {
    1219             if (iter->key->isSymbol() && !propertyNames.includeSymbolProperties())
     1228        if (iter->key->isSymbol()) {
     1229            foundSymbol = true;
     1230            if (propertyNames.propertyNameMode() != PropertyNameMode::Symbols)
    12201231                continue;
    1221             if (knownUnique)
    1222                 propertyNames.addUnchecked(iter->key);
    1223             else
    1224                 propertyNames.add(iter->key);
     1232        }
     1233        checkDontEnumAndAdd(iter);
     1234    }
     1235
     1236    if (foundSymbol && propertyNames.propertyNameMode() == PropertyNameMode::StringsAndSymbols) {
     1237        // To ensure the order defined in the spec, we append symbols at the last elements of keys.
     1238        // https://tc39.es/ecma262/#sec-ordinaryownpropertykeys
     1239        for (PropertyTable::iterator iter = table->begin(); iter != end; ++iter) {
     1240            if (iter->key->isSymbol())
     1241                checkDontEnumAndAdd(iter);
    12251242        }
    12261243    }
Note: See TracChangeset for help on using the changeset viewer.