Bug 185337 – Support instantiating a String with a StaticStringImpl{&, *}

• Status: ASSIGNED
• Component: Web Template Framework

[Daniel Bates]

We should support instantiating a String with a StaticStringImpl{&, *} so that we can construct constexpr StaticStringImpls and pass them to functions the take a String without needing to cast it to non-const StaticStringImpl beforehand. It is safe for String to take a const StaticStringImpl and internally cast to a non-const StringImpl because StaticStringImpls are guaranteed to be immutable. This immutability is guaranteed both at runtime (by [1]) and by convention of only instantiating a StaticStringImpl in a constexpr expression.

[1] <https://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/StringImpl.h?rev=231268#L330>

[Daniel Bates]

Created attachment 339622 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 339622 [details]
Patch

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

> Source/WTF/ChangeLog:11
> +        convention of only instantiating a StaticStringImpl in a constexpr expression.

Is this condition maintained for the refcount of StaticStringImpl? StaticStringImpl sets s_refCountFlagIsStaticString = 0x1.
Since our StringImpl increments/decrements the refcount by 2, setting 0x1 initial ensures that the refcount never becomes zero even if multiple threads ref/deref it.
Even though, the refcount itself is mutated.

[Daniel Bates]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 339622 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339622&action=review
> 
> > Source/WTF/ChangeLog:11
> > +        convention of only instantiating a StaticStringImpl in a constexpr expression.
> 
> Is this condition maintained for the refcount of StaticStringImpl?
> StaticStringImpl sets s_refCountFlagIsStaticString = 0x1.
> Since our StringImpl increments/decrements the refcount by 2, setting 0x1
> initial ensures that the refcount never becomes zero even if multiple
> threads ref/deref it.
> Even though, the refcount itself is mutated.

:( This patch is wrong.

[Daniel Bates]

(In reply to Yusuke Suzuki from comment #2)
> Comment on attachment 339622 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339622&action=review
> 
> > Source/WTF/ChangeLog:11
> > +        convention of only instantiating a StaticStringImpl in a constexpr expression.
> 
> Is this condition maintained for the refcount of StaticStringImpl?
> StaticStringImpl sets s_refCountFlagIsStaticString = 0x1.
> Since our StringImpl increments/decrements the refcount by 2, setting 0x1
> initial ensures that the refcount never becomes zero even if multiple
> threads ref/deref it.

How is it thread-safe to increment and decrement the ref count for a StringImpl? I mean, its ref count is an unsigned and we do not use we do not use locks in StringImpl::ref/deref(). In particular, how is StringImpl::deref() thread-safe?

[Yusuke Suzuki]

Comment on attachment 339622 [details]
Patch

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

>>>> Source/WTF/ChangeLog:11
>>>> +        convention of only instantiating a StaticStringImpl in a constexpr expression.
>>> 
>>> Is this condition maintained for the refcount of StaticStringImpl? StaticStringImpl sets s_refCountFlagIsStaticString = 0x1.
>>> Since our StringImpl increments/decrements the refcount by 2, setting 0x1 initial ensures that the refcount never becomes zero even if multiple threads ref/deref it.
>>> Even though, the refcount itself is mutated.
>> 
>> :( This patch is wrong.
> 
> How is it thread-safe to increment and decrement the ref count for a StringImpl? I mean, its ref count is an unsigned and we do not use we do not use locks in StringImpl::ref/deref(). In particular, how is StringImpl::deref() thread-safe?

Basic StringImpl is not thread-safe. When moving it around threads, we call `isolateCopy()`.
The exception is *static* StringImpl (like, StaticStringImpl. The StringImpl with `isStatic()` flag). In this case, the ref count is set to 0x1.
Since there is not thread-safety in StringImpl, the effect of StringImpl::ref/deref() would be lost. But it is OK since Static StringImpl is never destroyed. Threads increment/decrement refcount by 2. (ref += 2 / ref -= 2).
But refcount's first bit is always 0x1 in static StringImpl. So while the value of refCount() of static StringImpl becomes bogus, the refcount never becomes zero, and static StringImpl is never destroyed.