Bug 173230 – [WTF] Add RegisteredSymbolImpl

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[WTF] Add RegisteredSymbolImpl

[Yusuke Suzuki]

Created attachment 312577 [details]
Patch

[Mark Lam]

Comment on attachment 312577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312577&action=review

Nice.  r=me with some suggestions.

> Source/WTF/ChangeLog:18
> +        So many symbols are not registered in SymbolRegistry. However, now we always
> +        allocate a pointer member to point SymbolRegistry = nullptr. Since SymbolImpl
> +        is immutable, and it is not registered to SymbolRegistry after it is created,
> +        this member is wasteful.
> +
> +        Instead, we add a new derived class RegisteredSymbolImpl. SymbolImpl has a
> +        flag to indicate that it is registered to a SymbolRegistry. In SymbolRegistry,
> +        we create a RegisteredSymbolImpl and set itself to this symbol. By doing so,
> +
> +        1. We do not set m_symbolRegistry after creating a Symbol. It is clean.
> +        2. We reduce the size of SymbolImpl.

Some edits:

        Most symbols are not registered in SymbolRegistry. However, we currently always
        allocate a pointer member to point to a SymbolRegistry, and this pointer is always null.
        Since SymbolImpl is immutable, and it will never be registered with a SymbolRegistry
        after it is created. Hence, this member is wasteful.

        Instead, we add a new derived class RegisteredSymbolImpl, which will set a flag in
        SymbolImpl to indicate that it is registered with a SymbolRegistry. The only way to
        create a RegisteredSymbolImpl is via a factory method provided by the SymbolRegistry.
        The factory method will pass itself to the RegisteredSymbolImpl's constructor for
        initializing the RegisteredSymbolImpl's m_symbolRegistry field. By doing so,

        1. We do not need to set m_symbolRegistry after creating a Symbol. It is clean.
        2. We reduce the size of SymbolImpl.

> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:126
> +    return JSValue::encode(jsString(exec, vm.symbolRegistry().keyForSymbol(static_cast<RegisteredSymbolImpl&>(uid))));

Replace cast with *uid.asRegisteredSymbolImpl().  See below.

> Source/WTF/wtf/text/StringImpl.cpp:117
> -    if (isAtomic() && length() && !isSymbol())
> +    if (isAtomic() && length())
>          AtomicStringImpl::remove(static_cast<AtomicStringImpl*>(this));
>  
>      if (isSymbol()) {

From grepping the code, I see that this was always the case i.e. symbols are never atomic strings.  Can you just add an ASSERT(isSymbol()) in this if statement just in case anyone breaks this?

You can even rewrite this as:
    if (isAtomic()) {
        ASSERT(!isSymbol());
        if (length())
            AtomicStringImpl::remove(static_cast<AtomicStringImpl*>(this));
    } else if (isSymbol()) {

... because we shouldn't have to do the isSymbol() check if isAtomic().

> Source/WTF/wtf/text/StringImpl.cpp:121
> +            symbolRegistry->remove(static_cast<RegisteredSymbolImpl&>(symbol));

Replace cast with *symbol.asRegisteredSymbolImpl().  See below.

> Source/WTF/wtf/text/SymbolImpl.cpp:51
> -        return adoptRef(*new SymbolImpl(rep.m_data8, rep.length(), *ownerRep));
> -    return adoptRef(*new SymbolImpl(rep.m_data16, rep.length(), *ownerRep));
> +        return adoptRef(*new SymbolImpl(rep.m_data8, rep.length(), *ownerRep, 0));
> +    return adoptRef(*new SymbolImpl(rep.m_data16, rep.length(), *ownerRep, 0));

I think using a default argument for flags (see below) is better than passing 0 here.  You can revert this change.

> Source/WTF/wtf/text/SymbolImpl.h:37
> +    static constexpr const unsigned s_flagIsNullSymbol = 0b01u;
> +    static constexpr const unsigned s_flagIsRegistered = 0b10u;

I suggest defining a type for the flags to better document where it's used e.g.:
    typedef unsigned Flags;
    static constexpr const Flags s_flagDefault = 0;
    static constexpr const Flags s_flagIsNullSymbol = 0b01u;
    static constexpr const Flags s_flagIsRegistered = 0b10u;

> Source/WTF/wtf/text/SymbolImpl.h:43
> +    SymbolRegistry* symbolRegistry() const;

I suggest adding a method after this:

    RegisteredSymbolImpl* asRegisteredSymbolImpl()
    {
        ASSERT(isRegistered());
        return static_cast<RegisteredSymbolImpl*>(this);
    }

This way, you can do away with all the casts, and each time we cast, it asserts to make sure that it is safe to do so.

> Source/WTF/wtf/text/SymbolImpl.h:61
> +    SymbolImpl(const LChar* characters, unsigned length, Ref<StringImpl>&& base, unsigned flags)

I suggest changing this to:
    SymbolImpl(const LChar* characters, unsigned length, Ref<StringImpl>&& base, Flags flags = s_flagDefault)

> Source/WTF/wtf/text/SymbolImpl.h:70
> +    SymbolImpl(const UChar* characters, unsigned length, Ref<StringImpl>&& base, unsigned flags)

I suggest changing this to:
    SymbolImpl(const UChar* characters, unsigned length, Ref<StringImpl>&& base, Flags flags = s_flagDefault)

> Source/WTF/wtf/text/SymbolImpl.h:92
>      unsigned m_flags { 0 };

Change to:
    Flags m_flags { s_flagDefault };

> Source/WTF/wtf/text/SymbolRegistry.cpp:34
> +        static_cast<RegisteredSymbolImpl&>(*key.impl()).clearSymbolRegistry();

Replace with:
    key.impl().asRegisteredSymbolImpl()->clearSymbolRegistry();

> Source/WTF/wtf/text/SymbolRegistry.cpp:41
> +        return *static_cast<RegisteredSymbolImpl*>(addResult.iterator->impl());

Replace with:
    return *addResult.iterator->impl()->asRegisteredSymbolImpl();

[Mark Lam]

(In reply to Mark Lam from comment #2)
> Some edits:
> 
>         Most symbols are not registered in SymbolRegistry. However, we
> currently always
>         allocate a pointer member to point to a SymbolRegistry, and this
> pointer is always null.
>         Since SymbolImpl is immutable, and it will never be registered with
> a SymbolRegistry
>         after it is created. Hence, this member is wasteful.
> 
>         Instead, we add a new derived class RegisteredSymbolImpl, which will
> set a flag in
>         SymbolImpl to indicate that it is registered with a SymbolRegistry.
> The only way to
>         create a RegisteredSymbolImpl is via a factory method provided by
> the SymbolRegistry.
>         The factory method will pass itself to the RegisteredSymbolImpl's
> constructor for
>         initializing the RegisteredSymbolImpl's m_symbolRegistry field. By
> doing so,
> 
>         1. We do not need to set m_symbolRegistry after creating a Symbol.
> It is clean.
>         2. We reduce the size of SymbolImpl.
> 

Argg ... my edit needs some editing:

        Most symbols are not registered in SymbolRegistry. However, we currently always
        allocate a pointer member to point to a SymbolRegistry, and this pointer is always null.
        SymbolImpl is immutable, and it will never be registered with a SymbolRegistry
        after it is created. Hence, this member is wasteful.

        Instead, we add a new derived class RegisteredSymbolImpl, which will set a flag in
        SymbolImpl to indicate that it is registered with a SymbolRegistry. The only way to
        create a RegisteredSymbolImpl is via a factory method provided by the SymbolRegistry.
        The factory method will pass the SymbolRegistry this pointer to the RegisteredSymbolImpl's
        constructor for initializing the RegisteredSymbolImpl's m_symbolRegistry field.
        By doing so,

        1. We do not need to set m_symbolRegistry after creating a Symbol. It is clean.
        2. We reduce the size of SymbolImpls that do not need to be registered.

[Yusuke Suzuki]

Comment on attachment 312577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312577&action=review

Thank you.

>> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:126
>> +    return JSValue::encode(jsString(exec, vm.symbolRegistry().keyForSymbol(static_cast<RegisteredSymbolImpl&>(uid))));
> 
> Replace cast with *uid.asRegisteredSymbolImpl().  See below.

Fixed.

>> Source/WTF/wtf/text/StringImpl.cpp:117
>>      if (isSymbol()) {
> 
> From grepping the code, I see that this was always the case i.e. symbols are never atomic strings.  Can you just add an ASSERT(isSymbol()) in this if statement just in case anyone breaks this?
> 
> You can even rewrite this as:
>     if (isAtomic()) {
>         ASSERT(!isSymbol());
>         if (length())
>             AtomicStringImpl::remove(static_cast<AtomicStringImpl*>(this));
>     } else if (isSymbol()) {
> 
> ... because we shouldn't have to do the isSymbol() check if isAtomic().

Yup. Changed.

>> Source/WTF/wtf/text/StringImpl.cpp:121
>> +            symbolRegistry->remove(static_cast<RegisteredSymbolImpl&>(symbol));
> 
> Replace cast with *symbol.asRegisteredSymbolImpl().  See below.

Changed.

>> Source/WTF/wtf/text/SymbolImpl.cpp:51
>> +    return adoptRef(*new SymbolImpl(rep.m_data16, rep.length(), *ownerRep, 0));
> 
> I think using a default argument for flags (see below) is better than passing 0 here.  You can revert this change.

OK, dropped.

>> Source/WTF/wtf/text/SymbolImpl.h:37
>> +    static constexpr const unsigned s_flagIsRegistered = 0b10u;
> 
> I suggest defining a type for the flags to better document where it's used e.g.:
>     typedef unsigned Flags;
>     static constexpr const Flags s_flagDefault = 0;
>     static constexpr const Flags s_flagIsNullSymbol = 0b01u;
>     static constexpr const Flags s_flagIsRegistered = 0b10u;

Sounds fine. Fixed.

>> Source/WTF/wtf/text/SymbolImpl.h:43
>> +    SymbolRegistry* symbolRegistry() const;
> 
> I suggest adding a method after this:
> 
>     RegisteredSymbolImpl* asRegisteredSymbolImpl()
>     {
>         ASSERT(isRegistered());
>         return static_cast<RegisteredSymbolImpl*>(this);
>     }
> 
> This way, you can do away with all the casts, and each time we cast, it asserts to make sure that it is safe to do so.

Sounds nice.

>> Source/WTF/wtf/text/SymbolImpl.h:61
>> +    SymbolImpl(const LChar* characters, unsigned length, Ref<StringImpl>&& base, unsigned flags)
> 
> I suggest changing this to:
>     SymbolImpl(const LChar* characters, unsigned length, Ref<StringImpl>&& base, Flags flags = s_flagDefault)

Changed.

>> Source/WTF/wtf/text/SymbolImpl.h:70
>> +    SymbolImpl(const UChar* characters, unsigned length, Ref<StringImpl>&& base, unsigned flags)
> 
> I suggest changing this to:
>     SymbolImpl(const UChar* characters, unsigned length, Ref<StringImpl>&& base, Flags flags = s_flagDefault)

Changed.

>> Source/WTF/wtf/text/SymbolImpl.h:92
>>      unsigned m_flags { 0 };
> 
> Change to:
>     Flags m_flags { s_flagDefault };

Fixed.

>> Source/WTF/wtf/text/SymbolRegistry.cpp:34
>> +        static_cast<RegisteredSymbolImpl&>(*key.impl()).clearSymbolRegistry();
> 
> Replace with:
>     key.impl().asRegisteredSymbolImpl()->clearSymbolRegistry();

Fixed. But since key.impl() is StringImpl*, we need static_cast<SymbolImpl*>() anyway.

>> Source/WTF/wtf/text/SymbolRegistry.cpp:41
>> +        return *static_cast<RegisteredSymbolImpl*>(addResult.iterator->impl());
> 
> Replace with:
>     return *addResult.iterator->impl()->asRegisteredSymbolImpl();

Fixed.

[Yusuke Suzuki]

Committed r218066: <http://trac.webkit.org/changeset/218066>

[Mark Lam]

Comment on attachment 312577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312577&action=review

>>> Source/WTF/wtf/text/SymbolRegistry.cpp:34
>>> +        static_cast<RegisteredSymbolImpl&>(*key.impl()).clearSymbolRegistry();
>> 
>> Replace with:
>>     key.impl().asRegisteredSymbolImpl()->clearSymbolRegistry();
> 
> Fixed. But since key.impl() is StringImpl*, we need static_cast<SymbolImpl*>() anyway.

Sorry for my mistake, but you can still avoid the cast like so:
     key.impl()->asRegisteredSymbolImpl()->clearSymbolRegistry();

[Mark Lam]

Comment on attachment 312577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=312577&action=review

>>>> Source/WTF/wtf/text/SymbolRegistry.cpp:34
>>>> +        static_cast<RegisteredSymbolImpl&>(*key.impl()).clearSymbolRegistry();
>>> 
>>> Replace with:
>>>     key.impl().asRegisteredSymbolImpl()->clearSymbolRegistry();
>> 
>> Fixed. But since key.impl() is StringImpl*, we need static_cast<SymbolImpl*>() anyway.
> 
> Sorry for my mistake, but you can still avoid the cast like so:
>      key.impl()->asRegisteredSymbolImpl()->clearSymbolRegistry();

Oh, nevermind.  I see what you meant: key.impl() returns a StringImpl*, not a SymbolImpl*.  Hence, sadly, we needed a cast anyway.