[Webkit-unassigned] [Bug 189991] [WTF] Add ExternalStringImpl, a StringImpl for user controlled buffers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=189991

--- Comment #4 from Yusuke Suzuki <yusukesuzuki at slowstart.org> ---
Comment on attachment 350861
  --> https://bugs.webkit.org/attachment.cgi?id=350861
Patch

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

I think this should be one type of BufferOwnership since this "External" is one of the buffer ownership.
We have BufferInternal = 0, BufferOwned = 1, and BufferSubstring = 2 now. So, we should add BufferExternal = 3, and add an appropriate destructor call for ~StringImpl.

> Source/WTF/wtf/text/ExternalStringImpl.cpp:55
> +ExternalStringImpl::ExternalStringImpl(const LChar* characters, unsigned length, ExternalStringImplFreeFunction&& free) 
> +    : StringImpl(characters, length, ConstructWithoutCopying)
> +    , m_free(WTFMove(free))
> +{
> +    ASSERT(m_free);
> +    m_hashAndFlags |= s_hashFlagIsExternal;
> +}
> +
> +ExternalStringImpl::ExternalStringImpl(const UChar* characters, unsigned length, ExternalStringImplFreeFunction&& free)
> +    : StringImpl(characters, length, ConstructWithoutCopying)
> +    , m_free(WTFMove(free))
> +{
> +    ASSERT(m_free);
> +    m_hashAndFlags |= s_hashFlagIsExternal;
> +}

Let's add BufferExternal instead.

> Source/WTF/wtf/text/StringImpl.cpp:126
> +    } else if (isExternal()) {
> +        static_cast<ExternalStringImpl*>(this)->FreeExternalBuffer(const_cast<LChar*>(m_data8), sizeInBytes());
>      }

Move this to BufferOwnership checking section. (below).

> Source/WTF/wtf/text/StringImpl.h:185
> +    // The bottom 7 bits in the hash are flags.
> +    // TODO(kobybo): Is there any difference in efficiency bitween 7 or 8 (CPU wise)? we only need 7
> +    static constexpr const unsigned s_flagCount = 8;

Since BufferOwnership is still 2bits (Internal = 0, Owned = 1, Substring = 2, External = 3), we do not need to change this part.

> Source/WTF/wtf/text/StringImpl.h:194
> +    static constexpr const unsigned s_hashFlagIsExternal = 1u << (s_flagStringKindCount + 2);

Let's remove this.

> Source/WTF/wtf/text/StringImpl.h:288
> +    bool isExternal() const { return m_hashAndFlags & s_hashFlagIsExternal; }

We can make it `bufferOwnership() == BufferExternal`.

> Tools/TestWebKitAPI/Tests/WTF/StringImpl.cpp:790
> +

Let's add tests for,

1. ExternalStringImpl can be a Atomic (BufferOwnership = External, StringKind = Atomic).
2. ExternalStringImpl cannot be a Symbol (when creating a symbol from ExternalStringImpl, we should create a new Symbol).

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180926/04691516/attachment.html>