[webkit-reviews] review granted: [Bug 173230] [WTF] Add RegisteredSymbolImpl : [Attachment 312577] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 173230: [WTF] Add RegisteredSymbolImpl
https://bugs.webkit.org/show_bug.cgi?id=173230

Attachment 312577: Patch

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




--- Comment #2 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 312577
  --> https://bugs.webkit.org/attachment.cgi?id=312577
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();