Changeset 269801 in webkit

Timestamp:

Nov 13, 2020, 2:32:01 PM (6 years ago)

Author:

commit-queue@webkit.org

Message:

[JSC] Use symbols as identifiers for class fields computed names storage
​https://bugs.webkit.org/show_bug.cgi?id=216172

Patch by Xan López <Xan Lopez> on 2020-11-13
Reviewed by Yusuke Suzuki.

Use private symbols for the property keys of the class fields with
computed names. This is cleaner than using raw numeric identifiers and
will be less cumbersome when we add static fields. It also prevents
potential collisions if other features want to store data in the class
scope.

JSTests:

• stress/class-fields-harmony.js: new test, make sure

setFunctionName works properly with computed fields.

Source/JavaScriptCore:

• bytecompiler/NodesCodegen.cpp:

(JSC::PropertyListNode::emitSaveComputedFieldName): adapt a comment.

• parser/Parser.cpp:

(JSC::Parser<LexerType>::parseClass): use private identifiers for computed fields property keys.
(JSC::Parser<LexerType>::parseInstanceFieldInitializerSourceElements): ditto.

• parser/ParserArena.cpp:

(JSC::IdentifierArena::makePrivateIdentifier): method to create a private identifier.

• parser/ParserArena.h:
• runtime/CachedTypes.cpp:

(JSC::CachedUniquedStringImplBase::encode): consider registered symbols, they are used by the parser now.
(JSC::CachedUniquedStringImplBase::decode const): ditto.

• runtime/VM.cpp:

(JSC::VM::VM):

• runtime/VM.h:

(JSC::VM::privateSymbolRegistry): create a private symbol registry too.

Source/WTF:

• wtf/text/SymbolImpl.cpp:

(WTF::RegisteredSymbolImpl::createPrivate): add a method to create a registered private symbol from a string key.

• wtf/text/SymbolImpl.h:
• wtf/text/SymbolRegistry.cpp:

(WTF::SymbolRegistry::symbolForKey): consider that we can hold private symbols now too.

• wtf/text/SymbolRegistry.h:

(WTF::SymbolRegistry::SymbolRegistry): new enum type for public/private symbols.

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/class-fields-harmony.js (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp (modified) (5 diffs)
• Source/JavaScriptCore/parser/Parser.cpp (modified) (4 diffs)
• Source/JavaScriptCore/parser/ParserArena.cpp (modified) (1 diff)
• Source/JavaScriptCore/parser/ParserArena.h (modified) (1 diff)
• Source/JavaScriptCore/runtime/CachedTypes.cpp (modified) (3 diffs)
• Source/JavaScriptCore/runtime/VM.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/VM.h (modified) (2 diffs)
• Source/WTF/ChangeLog (modified) (1 diff)
• Source/WTF/wtf/text/SymbolImpl.cpp (modified) (1 diff)
• Source/WTF/wtf/text/SymbolImpl.h (modified) (1 diff)
• Source/WTF/wtf/text/SymbolRegistry.cpp (modified) (2 diffs)
• Source/WTF/wtf/text/SymbolRegistry.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-11-13  Xan López  <xan@igalia.com>
+
+        [JSC] Use symbols as identifiers for class fields computed names storage
+        https://bugs.webkit.org/show_bug.cgi?id=216172
+
+        Reviewed by Yusuke Suzuki.
+
+        Use private symbols for the property keys of the class fields with
+        computed names. This is cleaner than using raw numeric identifiers and
+        will be less cumbersome when we add static fields. It also prevents
+        potential collisions if other features want to store data in the class
+        scope.
+
+        * stress/class-fields-harmony.js: new test, make sure
+        setFunctionName works properly with computed fields.
+
 2020-11-13  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/JSTests/stress/class-fields-harmony.js

@@ -907 +907 @@
 }
 
+// Additional tests by the WebKit project.
+
+{
+  let x = 0;
+  let y = 'foo';
+  let z = { name: 'test' };
+
+  let C = class {
+    [x] = () => {
+      return 2;
+    };
+    [y] = class {};
+    [z] = class D {};
+  }
+
+  let c = new C();
+  assertSame(c[x](), 2);
+  assertSame(c[x].name, '0');
+  assertSame(c[y].name, 'foo');
+  assertSame(c[z].name, 'D');
+}

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-11-13  Xan López  <xan@igalia.com>
+
+        [JSC] Use symbols as identifiers for class fields computed names storage
+        https://bugs.webkit.org/show_bug.cgi?id=216172
+
+        Reviewed by Yusuke Suzuki.
+
+        Use private symbols for the property keys of the class fields with
+        computed names. This is cleaner than using raw numeric identifiers and
+        will be less cumbersome when we add static fields. It also prevents
+        potential collisions if other features want to store data in the class
+        scope.
+
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::PropertyListNode::emitSaveComputedFieldName): adapt a comment.
+        * parser/Parser.cpp:
+        (JSC::Parser<LexerType>::parseClass): use private identifiers for computed fields property keys.
+        (JSC::Parser<LexerType>::parseInstanceFieldInitializerSourceElements): ditto.
+        * parser/ParserArena.cpp:
+        (JSC::IdentifierArena::makePrivateIdentifier): method to create a private identifier.
+        * parser/ParserArena.h:
+        * runtime/CachedTypes.cpp:
+        (JSC::CachedUniquedStringImplBase::encode): consider registered symbols, they are used by the parser now.
+        (JSC::CachedUniquedStringImplBase::decode const): ditto.
+        * runtime/VM.cpp:
+        (JSC::VM::VM):
+        * runtime/VM.h:
+        (JSC::VM::privateSymbolRegistry): create a private symbol registry too.
+
 2020-11-13  Sergey Rubanov  <chi187@gmail.com>
 

trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

@@ -803 +803 @@
     RefPtr<RegisterID> propertyExpr;
 
-    // The 'name' refers to a synthetic numeric variable name in the private name scope, where the property key is saved for later use.
+    // The 'name' refers to a synthetic private name in the class scope, where the property key is saved for later use.
     const Identifier& description = *node.name();
     Variable var = generator.variable(description);
@@ -4818 +4818 @@
 {
     RefPtr<RegisterID> value = generator.newTemporary();
+    bool shouldSetFunctionName = false;
 
     if (!m_assign)
@@ -4823 +4824 @@
     else {
         generator.emitNode(value.get(), m_assign);
-        if (m_ident && generator.shouldSetFunctionName(m_assign))
+        shouldSetFunctionName = generator.shouldSetFunctionName(m_assign);
+        if (m_ident && shouldSetFunctionName && m_type != DefineFieldNode::Type::ComputedName)
             generator.emitSetFunctionName(value.get(), *m_ident);
     }
@@ -4851 +4853 @@
 
         // For ComputedNames, the expression has already been evaluated earlier during evaluation of a ClassExprNode.
-        // Here, `m_ident` refers to an integer ID in a class lexical scope, containing the value already converted to an Expression.
+        // Here, `m_ident` refers to private symbol ID in a class lexical scope, containing the value already converted to an Expression.
         Variable var = generator.variable(*m_ident);
         ASSERT_WITH_MESSAGE(!var.local(), "Computed names must be stored in captured variables");
@@ -4859 +4861 @@
         RefPtr<RegisterID> privateName = generator.newTemporary();
         generator.emitGetFromScope(privateName.get(), scope.get(), var, ThrowIfNotFound);
+        if (shouldSetFunctionName)
+            generator.emitSetFunctionName(value.get(), privateName.get());
         generator.emitProfileType(privateName.get(), var, m_position, m_position + m_ident->length());
         generator.emitCallDefineProperty(generator.thisRegister(), privateName.get(), value.get(), nullptr, nullptr, BytecodeGenerator::PropertyConfigurable | BytecodeGenerator::PropertyWritable | BytecodeGenerator::PropertyEnumerable, m_position);

trunk/Source/JavaScriptCore/parser/Parser.cpp

@@ -2838 +2838 @@
 }
 
+static constexpr ASCIILiteral instanceComputedNamePrefix { "instanceComputedName"_s };
+
 template <typename LexerType>
 template <class TreeBuilder> TreeClassExpression Parser<LexerType>::parseClass(TreeBuilder& context, FunctionNameRequirements requirements, ParserClassInfo<TreeBuilder>& info)
@@ -2997 +2999 @@
 
             if (computedPropertyName) {
-                ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm, numComputedFields++);
+                ident = &m_parserArena.identifierArena().makePrivateIdentifier(m_vm, instanceComputedNamePrefix, numComputedFields++);
                 DeclarationResultMask declarationResult = classScope->declareLexicalVariable(ident, true);
                 ASSERT_UNUSED(declarationResult, declarationResult == DeclarationResult::Valid);
@@ -3078 +3080 @@
         JSTokenLocation fieldLocation = tokenLocation();
         const Identifier* ident = nullptr;
-        TreeExpression computedPropertyName = 0;
         DefineFieldNode::Type type = DefineFieldNode::Type::Name;
         switch (m_token.m_type) {
@@ -3102 +3103 @@
             next();
             break;
-        case OPENBRACKET:
+        case OPENBRACKET: {
             next();
-            computedPropertyName = parseAssignmentExpression(context);
+            TreeExpression computedPropertyName = parseAssignmentExpression(context);
             failIfFalse(computedPropertyName, "Cannot parse computed property name");
             handleProductionOrFail(CLOSEBRACKET, "]", "end", "computed property name");
-            ident = &m_parserArena.identifierArena().makeNumericIdentifier(m_vm, numComputedFields++);
+            ident = &m_parserArena.identifierArena().makePrivateIdentifier(m_vm, instanceComputedNamePrefix, numComputedFields++);
             type = DefineFieldNode::Type::ComputedName;
             break;
+        }
         default:
             if (m_token.m_type & KeywordTokenFlag)

trunk/Source/JavaScriptCore/parser/ParserArena.cpp

@@ -108 +108 @@
 }
 
+const Identifier& IdentifierArena::makePrivateIdentifier(VM& vm, ASCIILiteral prefix, unsigned identifier)
+{
+    String symbolName = makeString(prefix, identifier);
+    auto symbol = vm.privateSymbolRegistry().symbolForKey(symbolName);
+    m_identifiers.append(Identifier::fromUid(symbol));
+    return m_identifiers.last();
 }
+
+}

trunk/Source/JavaScriptCore/parser/ParserArena.h

@@ -53 +53 @@
         const Identifier& makeBigIntDecimalIdentifier(VM&, const Identifier&, uint8_t radix);
         const Identifier& makeNumericIdentifier(VM&, double number);
+        const Identifier& makePrivateIdentifier(VM&, ASCIILiteral, unsigned);
 
     public:

trunk/Source/JavaScriptCore/runtime/CachedTypes.cpp

@@ -703 +703 @@
         m_isAtomic = string.isAtom();
         m_isSymbol = string.isSymbol();
+        m_isRegistered = false;
         m_isWellKnownSymbol = false;
+        m_isPrivate = false;
         RefPtr<StringImpl> impl = const_cast<StringImpl*>(&string);
 
         if (m_isSymbol) {
             SymbolImpl* symbol = static_cast<SymbolImpl*>(impl.get());
+            m_isRegistered = symbol->isRegistered();
+            m_isPrivate = symbol->isPrivate();
             if (!symbol->isNullSymbol()) {
                 // We have special handling for well-known symbols.
-                if (!symbol->isPrivate()) {
+                if (!m_isPrivate) {
                     m_isWellKnownSymbol = true;
                     impl = symbol->substring(strlen("Symbol."));
@@ -743 +747 @@
 
             SymbolImpl* symbol;
-            if (m_isWellKnownSymbol)
-                symbol = decoder.vm().propertyNames->builtinNames().lookUpWellKnownSymbol(buffer, m_length);
+            VM& vm = decoder.vm();
+            if (m_isRegistered) {
+                String str(buffer, m_length);
+                if (m_isPrivate)
+                    symbol = static_cast<SymbolImpl*>(&vm.privateSymbolRegistry().symbolForKey(str).leakRef());
+                else
+                    symbol = static_cast<SymbolImpl*>(&vm.symbolRegistry().symbolForKey(str).leakRef());
+            } else if (m_isWellKnownSymbol)
+                symbol = vm.propertyNames->builtinNames().lookUpWellKnownSymbol(buffer, m_length);
             else
-                symbol = decoder.vm().propertyNames->builtinNames().lookUpPrivateName(buffer, m_length);
+                symbol = vm.propertyNames->builtinNames().lookUpPrivateName(buffer, m_length);
             RELEASE_ASSERT(symbol);
             String str = symbol;
@@ -774 +785 @@
     bool m_isWellKnownSymbol : 1;
     bool m_isAtomic : 1;
+    bool m_isRegistered : 1;
+    bool m_isPrivate : 1;
     unsigned m_length;
 };

trunk/Source/JavaScriptCore/runtime/VM.cpp

@@ -383 +383 @@
     , deferredWorkTimer(DeferredWorkTimer::create(*this))
     , m_atomStringTable(vmType == Default ? Thread::current().atomStringTable() : new AtomStringTable)
+    , m_privateSymbolRegistry(WTF::SymbolRegistry::Type::PrivateSymbol)
     , propertyNames(nullptr)
     , emptyList(new ArgList)

trunk/Source/JavaScriptCore/runtime/VM.h

@@ -715 +715 @@
     AtomStringTable* m_atomStringTable;
     WTF::SymbolRegistry m_symbolRegistry;
+    WTF::SymbolRegistry m_privateSymbolRegistry;
     CommonIdentifiers* propertyNames;
     const ArgList* emptyList;
@@ -726 +727 @@
     AtomStringTable* atomStringTable() const { return m_atomStringTable; }
     WTF::SymbolRegistry& symbolRegistry() { return m_symbolRegistry; }
+    WTF::SymbolRegistry& privateSymbolRegistry() { return m_privateSymbolRegistry; }
 
     Strong<JSBigInt> heapBigIntConstantOne;

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2020-11-13  Xan López  <xan@igalia.com>
+
+        [JSC] Use symbols as identifiers for class fields computed names storage
+        https://bugs.webkit.org/show_bug.cgi?id=216172
+
+        Reviewed by Yusuke Suzuki.
+
+        Use private symbols for the property keys of the class fields with
+        computed names. This is cleaner than using raw numeric identifiers and
+        will be less cumbersome when we add static fields. It also prevents
+        potential collisions if other features want to store data in the class
+        scope.
+
+        * wtf/text/SymbolImpl.cpp:
+        (WTF::RegisteredSymbolImpl::createPrivate): add a method to create a registered private symbol from a string key.
+        * wtf/text/SymbolImpl.h:
+        * wtf/text/SymbolRegistry.cpp:
+        (WTF::SymbolRegistry::symbolForKey): consider that we can hold private symbols now too.
+        * wtf/text/SymbolRegistry.h:
+        (WTF::SymbolRegistry::SymbolRegistry): new enum type for public/private symbols.
+
 2020-11-12  Darin Adler  <darin@apple.com>
 

trunk/Source/WTF/wtf/text/SymbolImpl.cpp

@@ -80 +80 @@
 }
 
+Ref<RegisteredSymbolImpl> RegisteredSymbolImpl::createPrivate(StringImpl& rep, SymbolRegistry& symbolRegistry)
+{
+    auto* ownerRep = (rep.bufferOwnership() == BufferSubstring) ? rep.substringBuffer() : &rep;
+    ASSERT(ownerRep->bufferOwnership() != BufferSubstring);
+    if (rep.is8Bit())
+        return adoptRef(*new RegisteredSymbolImpl(rep.m_data8, rep.length(), *ownerRep, symbolRegistry, s_flagIsRegistered | s_flagIsPrivate));
+    return adoptRef(*new RegisteredSymbolImpl(rep.m_data16, rep.length(), *ownerRep, symbolRegistry, s_flagIsRegistered | s_flagIsPrivate));
+}
+
 } // namespace WTF

trunk/Source/WTF/wtf/text/SymbolImpl.h

@@ -157 +157 @@
 
     static Ref<RegisteredSymbolImpl> create(StringImpl& rep, SymbolRegistry&);
-
-    RegisteredSymbolImpl(const LChar* characters, unsigned length, Ref<StringImpl>&& base, SymbolRegistry& registry)
-        : SymbolImpl(characters, length, WTFMove(base), s_flagIsRegistered)
+    static Ref<RegisteredSymbolImpl> createPrivate(StringImpl& rep, SymbolRegistry&);
+
+    RegisteredSymbolImpl(const LChar* characters, unsigned length, Ref<StringImpl>&& base, SymbolRegistry& registry, Flags flags = s_flagIsRegistered)
+        : SymbolImpl(characters, length, WTFMove(base), flags)
         , m_symbolRegistry(&registry)
     {
     }
 
-    RegisteredSymbolImpl(const UChar* characters, unsigned length, Ref<StringImpl>&& base, SymbolRegistry& registry)
-        : SymbolImpl(characters, length, WTFMove(base), s_flagIsRegistered)
+    RegisteredSymbolImpl(const UChar* characters, unsigned length, Ref<StringImpl>&& base, SymbolRegistry& registry, Flags flags = s_flagIsRegistered)
+        : SymbolImpl(characters, length, WTFMove(base), flags)
         , m_symbolRegistry(&registry)
     {

trunk/Source/WTF/wtf/text/SymbolRegistry.cpp

@@ -29 +29 @@
 namespace WTF {
 
+SymbolRegistry::SymbolRegistry(Type type)
+    : m_symbolType(type)
+{
+}
+
 SymbolRegistry::~SymbolRegistry()
 {
@@ -45 +50 @@
     }
 
-    auto symbol = RegisteredSymbolImpl::create(*rep.impl(), *this);
-    *addResult.iterator = SymbolRegistryKey(&symbol.get());
-    return symbol;
+    RefPtr<RegisteredSymbolImpl> symbol;
+    if (m_symbolType == Type::PrivateSymbol)
+        symbol = RegisteredSymbolImpl::createPrivate(*rep.impl(), *this);
+    else
+        symbol = RegisteredSymbolImpl::create(*rep.impl(), *this);
+
+    *addResult.iterator = SymbolRegistryKey(symbol.get());
+    return symbol.releaseNonNull();
 }
 

trunk/Source/WTF/wtf/text/SymbolRegistry.h

@@ -79 +79 @@
     WTF_MAKE_NONCOPYABLE(SymbolRegistry);
 public:
-    SymbolRegistry() = default;
+    enum class Type : uint8_t { PublicSymbol, PrivateSymbol };
+    WTF_EXPORT_PRIVATE SymbolRegistry(Type = Type::PublicSymbol);
     WTF_EXPORT_PRIVATE ~SymbolRegistry();
 
@@ -88 +89 @@
 private:
     HashSet<SymbolRegistryKey> m_table;
+    Type m_symbolType;
 };