Closed
Bug 1339481
Opened 7 years ago
Closed 7 years ago
Structured clone can be really slow
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: sfink)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
5.17 KB,
patch
|
kanru
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
Looks like most of the time is spent in JS engine.
Component: DOM → JavaScript Engine
Assignee | ||
Comment 2•7 years ago
|
||
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•7 years ago
|
||
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
Assignee | ||
Comment 4•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment 5•7 years ago
|
||
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+
Assignee | ||
Comment 6•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8837426 -
Attachment is obsolete: true
Comment 7•7 years ago
|
||
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+
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa0d76dc8080 Specialize writing bytes in structured clone, r=kanru
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fa0d76dc8080
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•