Structured clone can be really slow

RESOLVED FIXED in Firefox 54

Status

()

Core
JavaScript Engine
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Ehsan, Assigned: sfink)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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.
Looks like most of the time is spent in JS engine.
Component: DOM → JavaScript Engine
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?
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
Created attachment 8837426 [details] [diff] [review]
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.
Attachment #8837426 - Flags: feedback?(kchen)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
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.
Attachment #8837426 - Flags: feedback?(kchen) → feedback+
Created attachment 8837753 [details] [diff] [review]
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.
Attachment #8837753 - Flags: review?(kchen)
Attachment #8837426 - Attachment is obsolete: true
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.
Attachment #8837753 - Flags: review?(kchen) → review+

Comment 8

a year ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0d76dc8080
Specialize writing bytes in structured clone, r=kanru

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa0d76dc8080
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1341900
You need to log in before you can comment on or make changes to this bug.