Bug 189991 – [WTF] Add ExternalStringImpl, a StringImpl for user controlled buffers

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Koby]

Created attachment 350859 [details]
run-javascriptcore-tests output

Hi,
In node-jsc, to match v8's string api, I needed to be able to allocate JSStrings with buffers managed externally, meaning they are allocated by the api users and should be freed by them when the JSString is destructed.
To support this is JSC\WTF, I've added WTF::ExternalStringImpl, a subclass of StringImpl which calls a custom free function when it's destructed (and always creates the StringImpl with ConstructWithoutCopying).

Regarding tests:
- I ran TestWTF (on macOS). String related tests have passed, I only got a few failures regarding HashMap (which I guess are not related).
- I ran run-javascriptcore-tests (on macOS). There were a few failures but if I'm reading the output correctly I only was two "FAILED" FTL related tests. I've attached the output.
- According to https://webkit.org/contributing-code/, I should run run-webkit-tests for regression tests, but I still can't get it to run. It fails to run on macOS and Windows (I've created a bug for macOS), and I'm still trying to get everything compiled on an ubuntu machine (gtk build fails with an "internal compiler error", I'll create a bug for that). Is it really mandatory for every patch? If it is, I would love some advice on where to put my efforts into, as it's been a bit frustrating. This is the second patch from node-jsc I'm sending, and I have quite a few more, and I would love to get a working test setup so I could start sending them :)

Thanks
Koby

[Koby]

Created attachment 350861 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/text/ExternalStringImpl.cpp:32:  Inline functions should not be annotated with WTF_EXPORT_PRIVATE. Remove the macro, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WTF/wtf/text/ExternalStringImpl.cpp:37:  Inline functions should not be annotated with WTF_EXPORT_PRIVATE. Remove the macro, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 9 files


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

[Koby]

Note that I've got the following style error during webkit-patch upload:
ERROR: Source/WTF/wtf/text/ExternalStringImpl.cpp:32:  Inline functions should not be annotated with WTF_EXPORT_PRIVATE. Remove the macro, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WTF/wtf/text/ExternalStringImpl.cpp:37:  Inline functions should not be annotated with WTF_EXPORT_PRIVATE. Remove the macro, or move the inline function definition out-of-line.  [build/webcore_export] [4]

but those functions aren't inline.

[Yusuke Suzuki]

Comment on attachment 350861 [details]
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).

[Koby]

That's a good point, but what about the user supplied "free" function? In node-jsc I must be able to call a user supplied dispose function (when JSStrings are freed).

Just for reference, The the v8 apis I need "shim" are v8::String's NewExternalOneByte\NewExternalTwoByte, where the user supply the buffer and a dispose function (by providing an object which inherits from an abstract  External(One)ByteStringResource class).

[Yusuke Suzuki]

(In reply to Koby from comment #5)
> That's a good point, but what about the user supplied "free" function? In
> node-jsc I must be able to call a user supplied dispose function (when
> JSStrings are freed).

I suggest putting it in ~StringImpl().

if (ownership == BufferExternal) {
    static_cast<ExternalStringImpl*>(this)->FreeExternalBuffer(const_cast<LChar*>(m_data8), sizeInBytes());
    return;
}

> 
> Just for reference, The the v8 apis I need "shim" are v8::String's
> NewExternalOneByte\NewExternalTwoByte, where the user supply the buffer and
> a dispose function (by providing an object which inherits from an abstract 
> External(One)ByteStringResource class).

[Koby]

Oh got it, I thought you suggested to completely remove ExternalStringImpl in favor of BufferExternal. Makes sense, I'll make the changes and upload a new patch.

[Yusuke Suzuki]

Comment on attachment 350861 [details]
Patch

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

> Source/WTF/wtf/text/ExternalStringImpl.h:48
> +    ALWAYS_INLINE void FreeExternalBuffer(void* buffer, unsigned bufferSize)

Use `freeExternalBuffer` instead of `FreeExternalBuffer`.
https://webkit.org/code-style-guidelines/ would be nice document for WebKit coding style :)

[Yusuke Suzuki]

Comment on attachment 350861 [details]
Patch

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

> Source/WTF/wtf/text/ExternalStringImpl.h:53
> +    ExternalStringImplFreeFunction m_free;

And please do not forget to destroy this `ExternalStringImplFreeFunction` in ~StringImpl() :).

[Koby]

Created attachment 351000 [details]
Patch

[Koby]

Thanks for the review, added an updated patch :)
Note that the:
m_hashAndFlags = (m_hashAndFlags & ~s_hashMaskBufferOwnership) | BufferExternal;
is kind of ugly, but I didn't want to change StringImpl's constructors.

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/text/ExternalStringImpl.cpp:32:  Inline functions should not be annotated with WTF_EXPORT_PRIVATE. Remove the macro, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WTF/wtf/text/ExternalStringImpl.cpp:37:  Inline functions should not be annotated with WTF_EXPORT_PRIVATE. Remove the macro, or move the inline function definition out-of-line.  [build/webcore_export] [4]
Total errors found: 2 in 9 files


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

[Yusuke Suzuki]

Comment on attachment 351000 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 351000 [details]
Patch

Clearing flags on attachment: 351000

Committed r236599: <https://trac.webkit.org/changeset/236599>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44864906>