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
Created attachment 350861 [details] Patch
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.
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.
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).
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).
(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).
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.
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 :)
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() :).
Created attachment 351000 [details] Patch
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.
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.
Comment on attachment 351000 [details] Patch r=me
Comment on attachment 351000 [details] Patch Clearing flags on attachment: 351000 Committed r236599: <https://trac.webkit.org/changeset/236599>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44864906>