Skip to content

Commit

Permalink
SerializedScriptValue could be faster by atomizing strings early in s…
Browse files Browse the repository at this point in the history
…ome cases

https://bugs.webkit.org/show_bug.cgi?id=259267
rdar://112759042

Reviewed by Darin Adler.

Upon deserialization, in the `ObjectStartVisitMember` case, we would call
`readStringData()` which would allocate a String from the raw characters
and we would later construct a JSC::Identifier from it using
JSC::Identifier::fromString(), which would then atomize the string.

To avoid unnecessary string allocations in the case where the string is
already in the AtomString table, we now add a `ShouldAtomize` parameter
to `readStringData()`. Then the parameter is `Yes`, it now constructs an
AtomString right away instead of allocating a String. As a result, we
avoid unnecessary String allocations and the construction of a
JSC::Identifier is cheap using an already atomized string.

* Source/WebCore/bindings/js/SerializedScriptValue.cpp:
(WebCore::CloneSerializer::CloneSerializer):
(WebCore::CloneDeserializer::deserializeString):
(WebCore::CloneDeserializer::readString):
(WebCore::CloneDeserializer::readNullableString):
(WebCore::CloneDeserializer::readStringData):
(WebCore::CloneDeserializer::deserialize):

Canonical link: https://commits.webkit.org/266503@main
  • Loading branch information
cdumez committed Aug 2, 2023
1 parent 0c7db85 commit d2a72f5
Showing 1 changed file with 22 additions and 13 deletions.
35 changes: 22 additions & 13 deletions Source/WebCore/bindings/js/SerializedScriptValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,7 @@ class CloneSerializer : CloneBase {
: CloneBase(lexicalGlobalObject)
, m_buffer(out)
, m_blobHandles(blobHandles)
, m_emptyIdentifier(Identifier::fromString(lexicalGlobalObject->vm(), emptyString()))
, m_emptyIdentifier(Identifier::fromString(lexicalGlobalObject->vm(), emptyAtom()))
, m_context(context)
, m_sharedBuffers(sharedBuffers)
#if ENABLE(OFFSCREEN_CANVAS_IN_WORKERS)
Expand Down Expand Up @@ -2591,7 +2591,8 @@ SerializationReturnCode CloneSerializer::serialize(JSValue in)
class CloneDeserializer : CloneBase {
WTF_FORBID_HEAP_ALLOCATION;
public:
static String deserializeString(const Vector<uint8_t>& buffer)
enum class ShouldAtomize : bool { No, Yes };
static String deserializeString(const Vector<uint8_t>& buffer, ShouldAtomize shouldAtomize = ShouldAtomize::No)
{
if (buffer.isEmpty())
return String();
Expand All @@ -2609,7 +2610,7 @@ class CloneDeserializer : CloneBase {
bool is8Bit = length & StringDataIs8BitFlag;
length &= ~StringDataIs8BitFlag;
String str;
if (!readString(ptr, end, str, length, is8Bit))
if (!readString(ptr, end, str, length, is8Bit, shouldAtomize))
return String();
return str;
}
Expand Down Expand Up @@ -2931,15 +2932,18 @@ class CloneDeserializer : CloneBase {
return i;
}

static bool readString(const uint8_t*& ptr, const uint8_t* end, String& str, unsigned length, bool is8Bit)
static bool readString(const uint8_t*& ptr, const uint8_t* end, String& str, unsigned length, bool is8Bit, ShouldAtomize shouldAtomize)
{
if (length >= std::numeric_limits<int32_t>::max() / sizeof(UChar))
return false;

if (is8Bit) {
if ((end - ptr) < static_cast<int>(length))
return false;
str = String { ptr, length };
if (shouldAtomize == ShouldAtomize::Yes)
str = AtomString { ptr, length };
else
str = String { ptr, length };
ptr += length;
return true;
}
Expand All @@ -2949,7 +2953,10 @@ class CloneDeserializer : CloneBase {
return false;

#if ASSUME_LITTLE_ENDIAN
str = String(reinterpret_cast<const UChar*>(ptr), length);
if (shouldAtomize == ShouldAtomize::Yes)
str = AtomString(reinterpret_cast<const UChar*>(ptr), length);
else
str = String(reinterpret_cast<const UChar*>(ptr), length);
ptr += length * sizeof(UChar);
#else
UChar* characters;
Expand All @@ -2959,31 +2966,33 @@ class CloneDeserializer : CloneBase {
readLittleEndian(ptr, end, c);
characters[i] = c;
}
if (shouldAtomize == ShouldAtomize::Yes)
str = AtomString { str };
#endif
return true;
}

bool readNullableString(String& nullableString)
bool readNullableString(String& nullableString, ShouldAtomize shouldAtomize = ShouldAtomize::No)
{
bool isNull;
if (!read(isNull))
return false;
if (isNull)
return true;
CachedStringRef stringData;
if (!readStringData(stringData))
if (!readStringData(stringData, shouldAtomize))
return false;
nullableString = stringData->string();
return true;
}

bool readStringData(CachedStringRef& cachedString)
bool readStringData(CachedStringRef& cachedString, ShouldAtomize shouldAtomize = ShouldAtomize::No)
{
bool scratch;
return readStringData(cachedString, scratch);
return readStringData(cachedString, scratch, shouldAtomize);
}

bool readStringData(CachedStringRef& cachedString, bool& wasTerminator)
bool readStringData(CachedStringRef& cachedString, bool& wasTerminator, ShouldAtomize shouldAtomize = ShouldAtomize::No)
{
if (m_failed)
return false;
Expand All @@ -3006,7 +3015,7 @@ class CloneDeserializer : CloneBase {
bool is8Bit = length & StringDataIs8BitFlag;
length &= ~StringDataIs8BitFlag;
String str;
if (!readString(m_ptr, m_end, str, length, is8Bit)) {
if (!readString(m_ptr, m_end, str, length, is8Bit, shouldAtomize)) {
fail();
return false;
}
Expand Down Expand Up @@ -4742,7 +4751,7 @@ DeserializationResult CloneDeserializer::deserialize()
case ObjectStartVisitMember: {
CachedStringRef cachedString;
bool wasTerminator = false;
if (!readStringData(cachedString, wasTerminator)) {
if (!readStringData(cachedString, wasTerminator, ShouldAtomize::Yes)) {
if (!wasTerminator)
goto error;

Expand Down

0 comments on commit d2a72f5

Please sign in to comment.