1493150 - Optimize string allocation when deserializing structured clone data

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tom S [:evilpie]]

Attachment: Optimize structured cloning by reducing string allocations

The readString implementation always uses malloc, even for zero length strings. We can even optimize this for inline or static strings by reading the chars into an on-stack buffer with the maximum inline string size.

This reduces the runtime for a very string heavy benchmark from 615ms to 450ms.

As far as I know, for inline strings etc. I don't have to care about the null delimiter. This code is sort of similar to StringBuffer::finishString, but that code doesn't handle static strings. I am not sure how much sense it would make, but we could call JSFlatString::new_ directly instead of NewString. (I don't think we would need to deflate serialized data usually)

Comment 1

[Jan de Mooij [:jandem]]

Comment on attachment 9010932 [details] [diff] [review]
Optimize structured cloning by reducing string allocations

Review of attachment 9010932 [details] [diff] [review]:
-----------------------------------------------------------------

Good find, thanks for fixing!

I think we want something like InlineCharBuffer<CharT>:

https://searchfox.org/mozilla-central/rev/0640ea80fbc8d48f8b197cd363e2535c95a15eb3/js/src/builtin/String.cpp#119

That could replace both the inline-chars + malloc code. Maybe we can move InlineCharBuffer to a header file?

Comment 2

[Jan de Mooij [:jandem]]

Another option is StringBuffer (you could call reserve, ensureTwoByteChars, raw{Latin1,TwoByte}Begin, finishString).

Comment 3

[Tom S [:evilpie]]

Attachment: Optimize structured cloning by reducing string allocations

Comment 4

[Tom S [:evilpie]]

Attachment: Move InlineCharBuffer to a new header

Comment 5

[Tom S [:evilpie]]

Comment on attachment 9011537 [details] [diff] [review]
Optimize structured cloning by reducing string allocations

Inlining more code into InlineCharBuffer gives a slight win on my benchmark (~25ms) and makes this about as fast as my own approach, maybe even a bit faster.

I opted to call toStringDontDeflate instead of toString, because I am assuming most serialized strings are not inflated, I actually haven't checked this and I am not sure what kind of workloads I would have to try.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 9011538 [details] [diff] [review]
Move InlineCharBuffer to a new header

Review of attachment 9011538 [details] [diff] [review]:
-----------------------------------------------------------------

The size (138 KB) of this patch is misleading :)

I think we could even drop the "-inl" because it's the only header file we have?

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 9011537 [details] [diff] [review]
Optimize structured cloning by reducing string allocations

Review of attachment 9011537 [details] [diff] [review]:
-----------------------------------------------------------------

Forwarding to anba because he wrote InlineCharBuffer<>.

Comment 8

[Tom S [:evilpie]]

Jan, can you review the other patch? I think we could also just drop the change to InlineCharBuffer, the perf difference is quite small.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9011537 [details] [diff] [review]
Optimize structured cloning by reducing string allocations

Review of attachment 9011537 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/StructuredClone.cpp
@@ +2167,5 @@
>          return nullptr;
>      }
> +
> +    if (nchars == 0) {
> +        return context()->names().empty;

I'm wondering if it makes sense to assert length != 0 in toString/toStringDontDeflate as it's a bit of a footgun...

Comment 10

[Tom S [:evilpie]]

> I think we could even drop the "-inl" because it's the only header file we have?
The include checker complains when I drop the -inl, because it includes another -inl header.

> I'm wondering if it makes sense to assert length != 0 in toString/toStringDontDeflate as it's a bit of a footgun...
I checked a few callers and most of them would violate this assert.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #10)
> I checked a few callers and most of them would violate this assert.

What do you think about handling it in toStringDontDeflate, like static strings? Or maybe use TryEmptyOrStaticString instead? The old code used that so it would be good to not regress the length == 0 case.

Comment 12

[Tom S [:evilpie]]

Attachment: v2 Optimize structured cloning by reducing string allocations

This is a great idea, I just had to move TryEmptyOrStaticString to a header as well.

I left the n == 0 check in JSStructuredCloneReader::readStringImpl because it skips a bunch of stuff, and might be relatively common.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 9014811 [details] [diff] [review]
v2 Optimize structured cloning by reducing string allocations

Review of attachment 9014811 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks.

::: js/src/vm/StructuredClone.cpp
@@ +2167,5 @@
>          return nullptr;
>      }
> +
> +    if (nchars == 0) {
> +        return context()->names().empty;

Can we remove this check now?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5748841757
Move InlineCharBuffer to a new header. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb3d036d145
Optimize structured cloning by reducing string allocations. r=jandem

Comment 15

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7a5748841757
https://hg.mozilla.org/mozilla-central/rev/0bb3d036d145