1339481 - Structured clone can be really slow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[(no longer active)]

See <https://clptr.io/2lg8WUl> where we're spending about 300ms running JSStructuredCloneWriter::write().

This may be related to bug 1339480, where we're postMessaging something huge to a worker.  The reason I suspect this is related to bug 1339480 is that it appears immediately following the sessionstore stuff there, so I suspect this is a promise being resolved from that code.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Looks like most of the time is spent in JS engine.

Comment 2

[Steve Fink [:sfink] [:s:]]

The thing that pops out to me is the 136ms spent in writeArray<unsigned char>. It's looping 1 byte at a time over buf.WriteBytes(reinterpret_cast<char*>(&value), sizeof(value). I don't know if it is able to optimize all that away -- it sort of looks like it is, given that it has a call to _platform_memmove in there, except that if that were true, then the majority of the time ought to be in the memmove, and it isn't.

So we could try manually specializing that instantiation.

Alternatively, it appears that that writeArray must be coming from writeBytes, which probably means we're either copying a string or an arraybuffer. In the string case, we might want to consider using external strings and passing by pointer. In the arraybuffer case, I wonder if this could be done via Transferring. What's the test case?

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

When porting JSStructuredClone to BufferList, I changed the zero padding method in WriteArray to a loop, maybe that's slow? I can add a memset method to BufferList and see if that helps.

http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/js/src/vm/StructuredClone.cpp#888-895

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Specialize writing bytes in structured clone

Ok, this was a little more involved than I expected, and isn't really complete.

I specialized for uint8_t, which is much simpler. But there was a bit of a chain reaction -- swapToLittleEndian<uint8_t> was no longer used, but if you delete it, there's no need for the local function at all. And I also did the padding in one go; I'm not sure if it'll be faster or slower. (It has to memset(0) a chunk of the stack, though it can do it with an 8-byte immediate on some architectures, then copy it over.) But I still think this is too trivial; I just changed it because it seemed simpler to me.

But that's where I ran into trouble. I reused nearly the same padding calculation in the general writeArray, but that was written in a very odd way, apparently because of the possibility of overflow. I replaced it with something that seems simpler and still overflow-safe to me, but I'm not 100% sure.

And really, if I change this logic I ought to change the matching logic in readArray. I guess I'd like someone to read it through once first, to make sure I'm not changing anything. Note that if you ever did writeArray<T> where sizeof(T) doesn't evenly divide sizeof(uint64_t) (ie, 8), then you'd get some weird stuff -- eg if sizeof(T) were 3, you would have n+4 bytes of padding at the end. It's enough to give each pair of 3-byte values their own 8-byte word, except we would pack them in tightly so it doesn't make sense. I suspect the original version wrote them more loosely -- but we never instantiate with anything weird like that. Even typed arrays of floats end up getting stored in either 32-bit or 64-bit chunks.

Before requesting review, I'll add some sort of brief comment about this.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8837426 [details] [diff] [review]
Specialize writing bytes in structured clone

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

(In reply to Steve Fink [:sfink] [:s:] from comment #4)
> And really, if I change this logic I ought to change the matching logic in
> readArray. I guess I'd like someone to read it through once first, to make
> sure I'm not changing anything. Note that if you ever did writeArray<T>
> where sizeof(T) doesn't evenly divide sizeof(uint64_t) (ie, 8), then you'd
> get some weird stuff -- eg if sizeof(T) were 3, you would have n+4 bytes of
> padding at the end. It's enough to give each pair of 3-byte values their own
> 8-byte word, except we would pack them in tightly so it doesn't make sense.
> I suspect the original version wrote them more loosely -- but we never
> instantiate with anything weird like that. Even typed arrays of floats end
> up getting stored in either 32-bit or 64-bit chunks.

We assert that T must evenly divide sizeof(uint64_t) so you don't have to worry the weird size case.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Specialize writing bytes in structured clone

Using CheckedInt removed most of the need for clever thinking. This patch also fixes a problem: if buf.writeBytes() failed, it was returning false without setting an exception.

Comment 7

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8837753 [details] [diff] [review]
Specialize writing bytes in structured clone

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

r+ with nits addressed.

::: js/src/vm/StructuredClone.cpp
@@ +876,5 @@
> +        if (!buf.WriteBytes(reinterpret_cast<char*>(&value), sizeof(value))) {
> +            ReportAllocationOverflow(context());
> +            return false;
> +        }
> +    }

buf (BufferList) uses js::TempAllocPolicy so it shouldn't need manual ReportAllocationOverflow or ReportOutOfMemory report.

@@ +886,3 @@
>          ReportAllocationOverflow(context());
>          return false;
>      }

Same here.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0d76dc8080
Specialize writing bytes in structured clone, r=kanru

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fa0d76dc8080