Bug 144848 – Introduce UniquedStringImpl and SymbolImpl to separate symbolic strings from AtomicStringImpl

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Yusuke Suzuki]

The current AtomicStringImpl accidentally means the symbol OR atomic StringImpl.
It's not correct to its name and it's error prone.

In this issue, we'll introduce new 2 classes into WTF.

1. UniquedStringImpl

It's derived class from StringImpl. And it represents symbol || atomic StringImpl.
Since it contains symbol string, prohibiting the operator== for the other strings is preferable I think.
Only pointer based comparison is accepted. (due to its uniqueness)

2. SymbolImpl

It's derived class from UniquedStringImpl. Only symbol strings can become this.
It ensures the given StringImpl is symbol in compile time.


And changing AtomicStringImpl.

3. AtomicStringImpl

It's derived class from UniquedStringImpl. Only atomic (non-normal && non-symbol) strings can become this.
It ensures the given StringImpl is atomic in compile time.

[Yusuke Suzuki]

Created attachment 252830 [details]
Prototype

JSC & WTF fix. WebCore and the others still remain

[Yusuke Suzuki]

Created attachment 252840 [details]
Prototype

OSX remains

[WebKit Commit Bot]

Attachment 252840 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:337:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/JavaScriptCore/runtime/TypeSet.h:59:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 78 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 252840 [details]
Prototype

Attachment 252840 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5711213828767744

New failing tests:
fast/dom/named-items-with-symbol-name.html

[Build Bot]

Created attachment 252842 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 252840 [details]
Prototype

Attachment 252840 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6274163782189056

New failing tests:
fast/dom/named-items-with-symbol-name.html

[Build Bot]

Created attachment 252844 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Yusuke Suzuki]

We need to create UniquedString, SymbolString in addition to AtomicString...

[Darin Adler]

Comment on attachment 252830 [details]
Prototype

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

Comments on an older version of the patch.

> Source/JavaScriptCore/builtins/BuiltinNames.h:97
> +inline bool BuiltinNames::isPrivateName(SymbolImpl* uid) const
> +{
> +    return m_privateToPublicMap.contains(uid);
> +}

This can’t work with null -- hash table operations just assert with null -- so the argument should be a reference, not a pointer.

> Source/JavaScriptCore/builtins/BuiltinNames.h:101
> +inline bool BuiltinNames::isPrivateName(UniquedStringImpl* uid) const
>  {
>      if (!uid->isSymbol())

This doesn’t work with null -- we dereference the pointer right away -- so the argument should be a reference, not a pointer.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3813
> +            return String(static_pointer_cast<StringImpl>(ptr->key));

I don’t understand the need to cast here.

> Source/JavaScriptCore/runtime/Identifier.h:178
> +    Identifier(SymbolImpl* uid) : m_string(uid) { ASSERT(m_string.impl()->isSymbol()); }

Should this constructor be explicit? Since this won’t work with null, this should take a reference, not a pointer. I don’t think the assertion makes sense now that the type is specific.

> Source/JavaScriptCore/runtime/MapData.h:118
> +    typedef HashMap<SymbolImpl*, int32_t, typename WTF::DefaultHash<SymbolImpl*>::Hash, WTF::HashTraits<SymbolImpl*>, IndexTraits> SymbolKeyedMap;

I think PtrHash would be clearer than DefaultHash here.

> Source/JavaScriptCore/runtime/PrivateName.h:30
>  #include <wtf/text/StringImpl.h>
> +#include <wtf/text/SymbolImpl.h>

Do we need both? I think that SymbolImpl.h must include StringImpl.h so we should just include it.

> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:33
> +#include <wtf/text/UniquedStringImpl.h>

Should not need to add this include, since AtomicStringImpl has to include UniquedStringImpl.

> Source/JavaScriptCore/runtime/PropertyName.h:40
>          ASSERT(!m_impl || m_impl->isAtomic() || m_impl->isSymbol());

Assertion seems unneeded now that the type indicates this at runtime.

> Source/JavaScriptCore/runtime/SymbolTable.h:43
>  #include <wtf/text/StringImpl.h>
> +#include <wtf/text/UniquedStringImpl.h>

Don’t think we need both of these includes.

> Source/WTF/wtf/PrintStream.h:75
>  inline void printInternal(PrintStream& out, const AtomicStringImpl* value) { printInternal(out, bitwise_cast<const StringImpl*>(value)); }
> +inline void printInternal(PrintStream& out, const UniquedStringImpl* value) { printInternal(out, bitwise_cast<const StringImpl*>(value)); }

Can’t we just do the StringImpl* one and leave out the overloads for derived classes from StringImpl?

> Source/WTF/wtf/text/AtomicStringImpl.h:24
>  #include <wtf/text/StringImpl.h>

Should not need this include.

> Source/WTF/wtf/text/AtomicStringImpl.h:35
>  public:
> -    AtomicStringImpl() : StringImpl(0) {}
> +    AtomicStringImpl()
> +        : UniquedStringImpl()
> +    {
> +    }

Should be able to just delete all of this.

> Source/WTF/wtf/text/StringImpl.h:137
> +    friend class UniquedStringImpl;
>      friend class AtomicStringImpl;
> +    friend class SymbolImpl;

Alphabetic sorting please? Why would AtomicStringImpl need to be a friend any more?

> Source/WTF/wtf/text/UniquedStringImpl.h:36
> +        : StringImpl(0)

Should be nullptr, not 0.

[Darin Adler]

(In reply to comment #8)
> We need to create UniquedString, SymbolString in addition to AtomicString...

We should not. We should just use RefPtr<UniquedStringImpl> and Ref<UniquedStringImpl> where we would otherwise use UniquedString. Same for SymbolStringImpl. Later we might rename the underlying classes to not have Impl in their names.

[Yusuke Suzuki]

Comment on attachment 252830 [details]
Prototype

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

>> Source/JavaScriptCore/builtins/BuiltinNames.h:97
>> +}
> 
> This can’t work with null -- hash table operations just assert with null -- so the argument should be a reference, not a pointer.

OK, I've changed it to reference.

>> Source/JavaScriptCore/builtins/BuiltinNames.h:101
>>      if (!uid->isSymbol())
> 
> This doesn’t work with null -- we dereference the pointer right away -- so the argument should be a reference, not a pointer.

Fixed.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:3813
>> +            return String(static_pointer_cast<StringImpl>(ptr->key));
> 
> I don’t understand the need to cast here.

Because there's no implict conversion from RefPtr<Derived> to RefPtr<Base>.
Here, I'll use ptr->key.get().

>> Source/JavaScriptCore/runtime/Identifier.h:178
>> +    Identifier(SymbolImpl* uid) : m_string(uid) { ASSERT(m_string.impl()->isSymbol()); }
> 
> Should this constructor be explicit? Since this won’t work with null, this should take a reference, not a pointer. I don’t think the assertion makes sense now that the type is specific.

Yeah! I've added explicit. (BTW, this constructor is private, and only called from the factory function.)
And drop the nonsense assert, it's now asserted in the compile phase by types!

>> Source/JavaScriptCore/runtime/MapData.h:118
>> +    typedef HashMap<SymbolImpl*, int32_t, typename WTF::DefaultHash<SymbolImpl*>::Hash, WTF::HashTraits<SymbolImpl*>, IndexTraits> SymbolKeyedMap;
> 
> I think PtrHash would be clearer than DefaultHash here.

Make sense!

>> Source/JavaScriptCore/runtime/PrivateName.h:30
>> +#include <wtf/text/SymbolImpl.h>
> 
> Do we need both? I think that SymbolImpl.h must include StringImpl.h so we should just include it.

Make sense. Just include SymbolImpl.h.

>> Source/JavaScriptCore/runtime/PropertyMapHashTable.h:33
>> +#include <wtf/text/UniquedStringImpl.h>
> 
> Should not need to add this include, since AtomicStringImpl has to include UniquedStringImpl.

Fixed

>> Source/JavaScriptCore/runtime/PropertyName.h:40
>>          ASSERT(!m_impl || m_impl->isAtomic() || m_impl->isSymbol());
> 
> Assertion seems unneeded now that the type indicates this at runtime.

Yeah. Fixed.

>> Source/JavaScriptCore/runtime/SymbolTable.h:43
>> +#include <wtf/text/UniquedStringImpl.h>
> 
> Don’t think we need both of these includes.

Change it to only including UniquedStringImpl.h.

>> Source/WTF/wtf/PrintStream.h:75
>> +inline void printInternal(PrintStream& out, const UniquedStringImpl* value) { printInternal(out, bitwise_cast<const StringImpl*>(value)); }
> 
> Can’t we just do the StringImpl* one and leave out the overloads for derived classes from StringImpl?

It raises the compile error since this is routed to the definition
template<typename T>
void printInternal(PrintStream& out, const T& value)
{
    value.dump(out);
}

To avoid it, I think we need to clean up this printInternal mechanism.
To make the patch simple I think using the current system in this patch is preferable; just adding UniquedStringImpl case such like AtomicStringImpl.

>> Source/WTF/wtf/text/AtomicStringImpl.h:24
>>  #include <wtf/text/StringImpl.h>
> 
> Should not need this include.

Fixed.

>> Source/WTF/wtf/text/AtomicStringImpl.h:35
>> +    }
> 
> Should be able to just delete all of this.

Yes, dropped.

>> Source/WTF/wtf/text/StringImpl.h:137
>> +    friend class SymbolImpl;
> 
> Alphabetic sorting please? Why would AtomicStringImpl need to be a friend any more?

This is because ~StringImpl() is private.
So I'll change these system.
1. Drop these friend classes
2. Drop these class' constructors

>> Source/WTF/wtf/text/UniquedStringImpl.h:36
>> +        : StringImpl(0)
> 
> Should be nullptr, not 0.

Dropped these constructors.

[Yusuke Suzuki]

(In reply to comment #10)
> (In reply to comment #8)
> > We need to create UniquedString, SymbolString in addition to AtomicString...
> 
> We should not. We should just use RefPtr<UniquedStringImpl> and
> Ref<UniquedStringImpl> where we would otherwise use UniquedString. Same for
> SymbolStringImpl. Later we might rename the underlying classes to not have
> Impl in their names.

I think it would make the patch very large.
Since we cannot change AtomicString to RefPtr<UniqueStringImpl> mechanically, because at least operator== should be considered.
So I suggest the following scenario.

1. In the separate patch, use RefPtr<UniqueStringImpl> instead of AtomicString.
2. In this patch, rename AtomicString into UniquedString.
3. AtomicString table related operations should be moved from AtomicString to AtomicStringImpl. (This makes sense because these static functions return StringImpl :D)
4. And UniquedString accept UniquedStringImpl without atomization.

When StringImpl / const char*, LChar* etc. are passed to UniquedString, it will be interned. But when UniquedStringImpl* comes, it will not interned. This design is correct to the name *uniqued*. And the change becomes simple. But we can introduce UniquedStringImpl, SymbolImpl and AtomicStringImpl. And this change paves the way to drop AtomicString and use UniqueStringImpl directly.

[Yusuke Suzuki]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > We need to create UniquedString, SymbolString in addition to AtomicString...
> > 
> > We should not. We should just use RefPtr<UniquedStringImpl> and
> > Ref<UniquedStringImpl> where we would otherwise use UniquedString. Same for
> > SymbolStringImpl. Later we might rename the underlying classes to not have
> > Impl in their names.
> 
> I think it would make the patch very large.
> Since we cannot change AtomicString to RefPtr<UniqueStringImpl>
> mechanically, because at least operator== should be considered.
> So I suggest the following scenario.
> 
> 1. In the separate patch, use RefPtr<UniqueStringImpl> instead of
> AtomicString.
> 2. In this patch, rename AtomicString into UniquedString.
> 3. AtomicString table related operations should be moved from AtomicString
> to AtomicStringImpl. (This makes sense because these static functions return
> StringImpl :D)
> 4. And UniquedString accept UniquedStringImpl without atomization.
> 
> When StringImpl / const char*, LChar* etc. are passed to UniquedString, it
> will be interned. But when UniquedStringImpl* comes, it will not interned.
> This design is correct to the name *uniqued*. And the change becomes simple.
> But we can introduce UniquedStringImpl, SymbolImpl and AtomicStringImpl. And
> this change paves the way to drop AtomicString and use UniqueStringImpl
> directly.

Still it requires significantly large changes.
So in this time, I just accept UniqueStringImpl in AtomicString.

As the result,

1. Now AtomicStringImpl issue is fixed. Its SymbolImpl is separated and UniquedStringImpl is introduced.
2. But AtomicString still have both symbol and atomic strings.

But I think it could be the first step to fix the things.

[Yusuke Suzuki]

(In reply to comment #10)
> (In reply to comment #8)
> > We need to create UniquedString, SymbolString in addition to AtomicString...
> 
> We should not. We should just use RefPtr<UniquedStringImpl> and
> Ref<UniquedStringImpl> where we would otherwise use UniquedString. Same for
> SymbolStringImpl. Later we might rename the underlying classes to not have
> Impl in their names.

And how to handle WTFString and StringImpl?

[Yusuke Suzuki]

Created attachment 252955 [details]
Patch

OSX remains

[Yusuke Suzuki]

Created attachment 252963 [details]
Patch

[WebKit Commit Bot]

Attachment 252963 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/AtomicString.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:48:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:50:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:166:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:169:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/TypeSet.h:59:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:122:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:123:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:337:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 15 in 91 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 252964 [details]
Patch

[WebKit Commit Bot]

Attachment 252964 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/AtomicString.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:48:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:50:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:166:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:169:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/TypeSet.h:59:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:122:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:123:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:337:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 15 in 94 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 252965 [details]
Patch

[WebKit Commit Bot]

Attachment 252965 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/AtomicString.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:47:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:48:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:49:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:50:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:60:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:63:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:166:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/AtomicString.h:169:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/TypeSet.h:59:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/WTF/wtf/text/WTFString.h:121:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:122:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/text/WTFString.h:123:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:337:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 15 in 94 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 252965 [details]
Patch

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

Added comments

> Source/WTF/wtf/text/AtomicString.h:50
> +    AtomicString(const UChar* s) : m_string(AtomicStringImpl::add(s)) { }

They are originaly written in one line. But style checker raises errors.
What do you think about it? Is it preferable to extract it such as the following?

AtomicString(const char* s)
    : m_string(add(s))
{
}

> Source/WTF/wtf/text/AtomicStringImpl.h:48
> +    }

They are renamed from `find` to `lookup`. Because C++ spec doesn't allow the static & non-static member functions that have the same signature.
StringImpl (the base class) already has non-static member function `find(StringImpl* string)`.

[Yusuke Suzuki]

Comment on attachment 252965 [details]
Patch

Ah, ok. ready for review.

[Yusuke Suzuki]

I'll split the AtomicStringImpl table part to the separate patch.

[Yusuke Suzuki]

Separated issue is opened. https://bugs.webkit.org/show_bug.cgi?id=145109

[Yusuke Suzuki]

Now, AtomicStringImpl table operation part is landed in the separate patch.
I'll rebase this patch now...

[Yusuke Suzuki]

*** Bug 144440 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Created attachment 253445 [details]
Patch

[WebKit Commit Bot]

Attachment 253445 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/TypeSet.cpp:337:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
ERROR: Source/JavaScriptCore/runtime/TypeSet.h:59:  The parameter type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 2 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 253448 [details]
Patch

[Yusuke Suzuki]

OK, the patch is ready for review.

[Darin Adler]

Comment on attachment 253448 [details]
Patch

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

Nice job.

I prefer auto or auto* or even possibly const auto* to stating the type when we just want to get something and don’t want to express an opinion about the type it should have. I don’t know if we have consensus about that, though.

> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2026
> +                    UniquedStringImpl* uid = ident.impl();

I think auto is better here than naming a class. I also think it’s not helpful to put this into a local variable. I also think auto is better for the iterator type below.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:453
> +        typedef HashMap<UniquedStringImpl*, GetterSetterPair, IdentifierRepHash> GetterSetterMap;

Behavior change here. Will this be a performance improvement?

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-3638
> -            AtomicStringImpl* uid;
> +            UniquedStringImpl* uid = nullptr;
>              if (identifierNumber != UINT_MAX)
>                  uid = m_graph.identifiers()[identifierNumber];
> -            else
> -                uid = nullptr;

Not sure that initializing to null is an improvement. Rationale for the change?

> Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp:74
> +        UniquedStringImpl* rep = m_addedIdentifiers[i];

auto is better

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:962
> +                UniquedStringImpl* uid = m_graph.identifiers()[node->identifierNumber()];

auto is better

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:909
> +                static_cast<const AtomicStringImpl*>(string->tryGetValueImpl()));

Does not look good! Why is this a safe cast? I don’t see a type check. May require a comment.

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:2092
> +        UniquedStringImpl* uid = m_graph.identifiers()[m_node->identifierNumber()];

auto

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:4816
> +                const AtomicStringImpl* str = static_cast<const AtomicStringImpl*>(string->tryGetValueImpl());

auto

> Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp:5739
> +        UniquedStringImpl* uid = m_graph.identifiers()[m_node->identifierNumber()];

auto

> Source/JavaScriptCore/parser/Nodes.h:1565
> -        bool captures(StringImpl* uid) { return m_capturedVariables.contains(uid); }
> +        bool captures(UniquedStringImpl* uid) { return m_capturedVariables.contains(uid); }

This function won’t work on null pointers because it uses the pointer as a key in hash table functions, so it should definitely take a reference, not a pointer, in the future.

> Source/JavaScriptCore/parser/Nodes.h:1613
> +        Vector<RefPtr<UniquedStringImpl>> m_closedVariables;

Seems like this should be Vector<Ref>, not Vector<RefPtr>.

> Source/JavaScriptCore/parser/Parser.h:108
>  struct ScopeLabelInfo {
> -    ScopeLabelInfo(StringImpl* ident, bool isLoop)
> +    ScopeLabelInfo(UniquedStringImpl* ident, bool isLoop)
>          : m_ident(ident)
>          , m_isLoop(isLoop)
>      {
>      }
>  
> -    StringImpl* m_ident;
> +    UniquedStringImpl* m_ident;
>      bool m_isLoop;
>  };

Wrong coding style for structs. Data member names should not have m_ prefix. Should not have a constructor; should use ScopeLabeInfo { identifier, isLoop } syntax instead when constructing.

> Source/JavaScriptCore/parser/Parser.h:890
> +    Vector<RefPtr<UniquedStringImpl>> m_closedVariables;

Should be Vector<Ref>, not Vector<RefPtr>.

> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:45
> +    Vector<RefPtr<UniquedStringImpl>> usedVariables;
> +    Vector<RefPtr<UniquedStringImpl>> writtenVariables;

Should be Vector<Ref>, not Vector<RefPtr>.

> Source/JavaScriptCore/runtime/Identifier.h:181
> +    explicit Identifier(SymbolImpl* uid)
> +        : m_string(uid)
> +    {
> +    }

Is null allowed? If not, this should change to take a SymbolImpl&.

> Source/JavaScriptCore/runtime/Identifier.h:277
> +    UniquedStringImpl* uid = identifier.impl();

auto

> Source/JavaScriptCore/runtime/IdentifierInlines.h:77
> +    return Identifier(static_cast<SymbolImpl*>(uid));

Surprised that Identifier() is needed. I would expect that just static_cast<SymbolImpl*>(uid) would work.

> Source/JavaScriptCore/runtime/IdentifierInlines.h:87
> +    return Identifier(name.uid());

Surprised that Identifier() is needed. I would expect that just name.uid() would work.

> Source/JavaScriptCore/runtime/Lookup.h:103
> +        UniquedStringImpl* uid = propertyName.uid();

auto

> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:245
> +        UniquedStringImpl* impl = properties[i].impl();

auto

> Source/JavaScriptCore/runtime/PrivateName.h:40
> +    explicit PrivateName(SymbolImpl* uid)

If null is not allowed, this should be a reference, not a pointer.

> Source/JavaScriptCore/runtime/PropertyName.h:37
> +    PropertyName(UniquedStringImpl* propertyName)

If null is not allowed, this should be a reference, not a pointer.

> Source/JavaScriptCore/runtime/PropertyName.h:118
> +    UniquedStringImpl* uid = propertyName.uid();

auto

> Source/JavaScriptCore/runtime/PropertyNameArray.h:72
> +    void add(UniquedStringImpl*);
> +    void addKnownUnique(UniquedStringImpl*);

Null is not allowed, so these arguments should be reference, not pointers.

> Source/JavaScriptCore/runtime/Structure.cpp:885
> +    UniquedStringImpl* rep = propertyName.uid();

auto

> Source/JavaScriptCore/runtime/Structure.cpp:904
> +    UniquedStringImpl* rep = propertyName.uid();

auto

> Source/JavaScriptCore/runtime/Structure.h:51
> -#include <wtf/text/AtomicStringImpl.h>
> +#include <wtf/text/UniquedStringImpl.h>

Does this file really require including that header? Would a forward declaration suffice? I don’t see code below actually operating on these objects, just moving pointers around.

> Source/JavaScriptCore/runtime/StructureTransitionTable.h:134
>      inline void add(VM&, Structure*);
> -    inline bool contains(AtomicStringImpl* rep, unsigned attributes) const;
> -    inline Structure* get(AtomicStringImpl* rep, unsigned attributes) const;
> +    inline bool contains(UniquedStringImpl* rep, unsigned attributes) const;
> +    inline Structure* get(UniquedStringImpl* rep, unsigned attributes) const;

inline here has no effect and should be removed. Argument name "rep" adds nothing. Arguments should probably be reference not pointer.

> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:112
> +    Ref<SymbolImpl> uid = exec->vm().symbolRegistry().symbolForKey(string);
> +    return JSValue::encode(Symbol::create(exec->vm(), &uid.get()));

Would read better as a one-liner.

> Source/JavaScriptCore/runtime/TypeSet.cpp:353
> +    for (auto key : m_fields) {

Should be auto&, not auto.

> Source/JavaScriptCore/runtime/TypeSet.h:59
> +    void addProperty(Ref<UniquedStringImpl>);

Wrong type here. This should just be UniquedStringImpl& or Ref&&; we get no value out of requiring the caller to pass in a Ref.

> Source/WTF/wtf/text/AtomicString.h:63
> +    // FIXME: AtomicString may have either SymbolImpl or AtomicStringImpl.

That FIXME is unclear. I would write:

    // FIXME: AtomicString doesn’t always have AtomicStringImpl, so one of those two names needs to change.

> Source/WTF/wtf/text/StringImpl.cpp:118
> +        symbolRegistry()->remove(*static_cast<SymbolImpl*>(this));

I would write static_cast<SymbolImpl&)(*this)

> Source/WTF/wtf/text/SymbolImpl.h:29
> +#include <wtf/text/StringImpl.h>

No reason for this include. Please don’t do it.

> Source/WTF/wtf/text/SymbolImpl.h:34
> +class SymbolImpl : public UniquedStringImpl {

Needs a comment to explain what this class is for and how it differs from AtomicStringImpl.

> Source/WTF/wtf/text/SymbolImpl.h:54
> +#if !ASSERT_DISABLED
> +// SymbolImpls created from StaticASCIILiteral will ASSERT
> +// in the generic ValueCheck<T>::checkConsistency
> +// as they are not allocated by fastMalloc.
> +// We don't currently have any way to detect that case
> +// so we ignore the consistency check for all SymbolImpls*.
> +template<> struct
> +ValueCheck<SymbolImpl*> {
> +    static void checkConsistency(const SymbolImpl*) { }
> +};
> +
> +template<> struct
> +ValueCheck<const SymbolImpl*> {
> +    static void checkConsistency(const SymbolImpl*) { }
> +};
> +#endif

Annoying that we are cut and pasted this code into three different header files. Would be good to find a nicer way to do this later.

> Source/WTF/wtf/text/SymbolRegistry.h:86
> +    WTF_EXPORT_PRIVATE String keyForSymbol(SymbolImpl& uid);

Argument name should be omitted here.

> Source/WTF/wtf/text/SymbolRegistry.h:88
> +    void remove(SymbolImpl& uid);

Argument name should be omitted here.

> Source/WTF/wtf/text/UniquedStringImpl.h:33
> +class UniquedStringImpl : public StringImpl {

Needs a comment to explain what this class is for and how it differs from StringImpl.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:309
> +        ASSERT(!propertyName.isSymbol());

What guarantees this? Comment needs to say why we think this will be true.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:310
> +        AtomicStringImpl* atomicPropertyName = static_cast<AtomicStringImpl*>(propertyName.impl());

auto

[Yusuke Suzuki]

Comment on attachment 253448 [details]
Patch

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

Thank you.
Almost all comments are fixed. But I think changing RefPtr => Ref should be done in a separate patch because it involves too much code fix since it involves IdentifierSet etc.
In the separate patch, we should consider about storing Ref to hash maps instead of RefPtr.

>> Source/JavaScriptCore/bytecode/CodeBlock.cpp:2026
>> +                    UniquedStringImpl* uid = ident.impl();
> 
> I think auto is better here than naming a class. I also think it’s not helpful to put this into a local variable. I also think auto is better for the iterator type below.

OK, here, just use `ident.impl()` directly.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:453
>> +        typedef HashMap<UniquedStringImpl*, GetterSetterPair, IdentifierRepHash> GetterSetterMap;
> 
> Behavior change here. Will this be a performance improvement?

Yeah. This should improve performance because this map doesn't check the content of the StringImpl*.
It just checks the pointer values.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:-3638
>> -                uid = nullptr;
> 
> Not sure that initializing to null is an improvement. Rationale for the change?

Personally, uninitialized local variable is not good I think.
But in this time, it's not important for this patch. So just using the previous code.

>> Source/JavaScriptCore/dfg/DFGDesiredIdentifiers.cpp:74
>> +        UniquedStringImpl* rep = m_addedIdentifiers[i];
> 
> auto is better

OK, use `auto`.

>> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:962
>> +                UniquedStringImpl* uid = m_graph.identifiers()[node->identifierNumber()];
> 
> auto is better

Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:909
>> +                static_cast<const AtomicStringImpl*>(string->tryGetValueImpl()));
> 
> Does not look good! Why is this a safe cast? I don’t see a type check. May require a comment.

This path is checked with the if statement. `if (string->tryGetValueImpl() && string->tryGetValueImpl()->isAtomic()) {`.
So when reaching here, it is guaranteed that the result of `string->tryGetValueImpl()` is always `AtomicStringImpl*`.
OK, I've added comment.

>> Source/JavaScriptCore/parser/Nodes.h:1565
>> +        bool captures(UniquedStringImpl* uid) { return m_capturedVariables.contains(uid); }
> 
> This function won’t work on null pointers because it uses the pointer as a key in hash table functions, so it should definitely take a reference, not a pointer, in the future.

I think it is better to fix these things in the separate patch because it involves so many changes across the code.

>> Source/JavaScriptCore/parser/Nodes.h:1613
>> +        Vector<RefPtr<UniquedStringImpl>> m_closedVariables;
> 
> Seems like this should be Vector<Ref>, not Vector<RefPtr>.

ditto. At that time, we should consider about storing Ref<UniquedStringImpl> in hash tables.

>> Source/JavaScriptCore/parser/Parser.h:108
>>  };
> 
> Wrong coding style for structs. Data member names should not have m_ prefix. Should not have a constructor; should use ScopeLabeInfo { identifier, isLoop } syntax instead when constructing.

Fixed.

>> Source/JavaScriptCore/parser/Parser.h:890
>> +    Vector<RefPtr<UniquedStringImpl>> m_closedVariables;
> 
> Should be Vector<Ref>, not Vector<RefPtr>.

Ditto. This involves all IdentifierSet changes.

>> Source/JavaScriptCore/parser/SourceProviderCacheItem.h:45
>> +    Vector<RefPtr<UniquedStringImpl>> writtenVariables;
> 
> Should be Vector<Ref>, not Vector<RefPtr>.

ditto.

>> Source/JavaScriptCore/runtime/Identifier.h:181
>> +    }
> 
> Is null allowed? If not, this should change to take a SymbolImpl&.

Nice catch. Fixed.

>> Source/JavaScriptCore/runtime/Identifier.h:277
>> +    UniquedStringImpl* uid = identifier.impl();
> 
> auto

Fixed.

>> Source/JavaScriptCore/runtime/IdentifierInlines.h:77
>> +    return Identifier(static_cast<SymbolImpl*>(uid));
> 
> Surprised that Identifier() is needed. I would expect that just static_cast<SymbolImpl*>(uid) would work.

OK, dropping `explicit` in private Identifier(SymbolImpl&).

>> Source/JavaScriptCore/runtime/IdentifierInlines.h:87
>> +    return Identifier(name.uid());
> 
> Surprised that Identifier() is needed. I would expect that just name.uid() would work.

Fixed.

>> Source/JavaScriptCore/runtime/Lookup.h:103
>> +        UniquedStringImpl* uid = propertyName.uid();
> 
> auto

Done.

>> Source/JavaScriptCore/runtime/ObjectConstructor.cpp:245
>> +        UniquedStringImpl* impl = properties[i].impl();
> 
> auto

Done.

>> Source/JavaScriptCore/runtime/PrivateName.h:40
>> +    explicit PrivateName(SymbolImpl* uid)
> 
> If null is not allowed, this should be a reference, not a pointer.

Changed it to SymbolImpl&.

>> Source/JavaScriptCore/runtime/PropertyName.h:37
>> +    PropertyName(UniquedStringImpl* propertyName)
> 
> If null is not allowed, this should be a reference, not a pointer.

Here, since it involves too much code change, changing it in this patch is not good I think.
I'll fix it in the separate patch.

>> Source/JavaScriptCore/runtime/PropertyName.h:118
>> +    UniquedStringImpl* uid = propertyName.uid();
> 
> auto

Fixed.

>> Source/JavaScriptCore/runtime/PropertyNameArray.h:72
>> +    void addKnownUnique(UniquedStringImpl*);
> 
> Null is not allowed, so these arguments should be reference, not pointers.

Ditto.

>> Source/JavaScriptCore/runtime/Structure.cpp:885
>> +    UniquedStringImpl* rep = propertyName.uid();
> 
> auto

Fixed.

>> Source/JavaScriptCore/runtime/Structure.cpp:904
>> +    UniquedStringImpl* rep = propertyName.uid();
> 
> auto

Fixed.

>> Source/JavaScriptCore/runtime/Structure.h:51
>> +#include <wtf/text/UniquedStringImpl.h>
> 
> Does this file really require including that header? Would a forward declaration suffice? I don’t see code below actually operating on these objects, just moving pointers around.

Changed it to forward declaration.

>> Source/JavaScriptCore/runtime/StructureTransitionTable.h:134
>> +    inline Structure* get(UniquedStringImpl* rep, unsigned attributes) const;
> 
> inline here has no effect and should be removed. Argument name "rep" adds nothing. Arguments should probably be reference not pointer.

Yeah, that's reasonable. Dropped inline and `rep`. And dropped `inline` in Structure.cpp.

>> Source/JavaScriptCore/runtime/SymbolConstructor.cpp:112
>> +    return JSValue::encode(Symbol::create(exec->vm(), &uid.get()));
> 
> Would read better as a one-liner.

Fixed.

>> Source/JavaScriptCore/runtime/TypeSet.cpp:353
>> +    for (auto key : m_fields) {
> 
> Should be auto&, not auto.

Oops, thank you!

>> Source/JavaScriptCore/runtime/TypeSet.h:59
>> +    void addProperty(Ref<UniquedStringImpl>);
> 
> Wrong type here. This should just be UniquedStringImpl& or Ref&&; we get no value out of requiring the caller to pass in a Ref.

Fixed!

>> Source/WTF/wtf/text/AtomicString.h:63
>> +    // FIXME: AtomicString may have either SymbolImpl or AtomicStringImpl.
> 
> That FIXME is unclear. I would write:
> 
>     // FIXME: AtomicString doesn’t always have AtomicStringImpl, so one of those two names needs to change.

Looks very nice. Changed.

>> Source/WTF/wtf/text/StringImpl.cpp:118
>> +        symbolRegistry()->remove(*static_cast<SymbolImpl*>(this));
> 
> I would write static_cast<SymbolImpl&)(*this)

Fixed.

>> Source/WTF/wtf/text/SymbolImpl.h:29
>> +#include <wtf/text/StringImpl.h>
> 
> No reason for this include. Please don’t do it.

Oops. Dropped.

>> Source/WTF/wtf/text/SymbolImpl.h:34
>> +class SymbolImpl : public UniquedStringImpl {
> 
> Needs a comment to explain what this class is for and how it differs from AtomicStringImpl.

Added comment.

>> Source/WTF/wtf/text/SymbolImpl.h:54
>> +#endif
> 
> Annoying that we are cut and pasted this code into three different header files. Would be good to find a nicer way to do this later.

Yeah, something like StringHasher.h is needed for these traits.

>> Source/WTF/wtf/text/SymbolRegistry.h:86
>> +    WTF_EXPORT_PRIVATE String keyForSymbol(SymbolImpl& uid);
> 
> Argument name should be omitted here.

Fixed.

>> Source/WTF/wtf/text/SymbolRegistry.h:88
>> +    void remove(SymbolImpl& uid);
> 
> Argument name should be omitted here.

Fixed.

>> Source/WTF/wtf/text/UniquedStringImpl.h:33
>> +class UniquedStringImpl : public StringImpl {
> 
> Needs a comment to explain what this class is for and how it differs from StringImpl.

Added comment.

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:309
>> +        ASSERT(!propertyName.isSymbol());
> 
> What guarantees this? Comment needs to say why we think this will be true.

This propertyName is generated from `Identifier::from`. This function generates Identifier from String and it always returns non-symbol Identifier.

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:310
>> +        AtomicStringImpl* atomicPropertyName = static_cast<AtomicStringImpl*>(propertyName.impl());
> 
> auto

Fixed.

[Yusuke Suzuki]

Committed r184828: <http://trac.webkit.org/changeset/184828>