1341902 - js::Stringify can be really slow on Google Drive

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[(no longer active)]

I got this on <https://drive.google.com/drive/u/1/folders/0BzDhuKIxRtxdY2NucFRIQWZkZlU>.

https://perfht.ml/2kNDQ7f

We're spending 554ms in js::Stringify.

Comment 1

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

js::Stringify converts values to JSON. About half of the time is spent calling back into JS. I don't know if that's the replacer callback or just ToString calls, probably the latter. 61ms of the time is getting properties.

I'm not sure if this is just a matter of producing a ton of JSON, or whether we're actually slow here.

Comment 2

[Tom S [:evilpie]]

I saved on of the resulting JSON strings locally, turned it back into an object and parsed it a few times with JSON.parse. We seem to spend about 30% of our time in the CycleDetector! perf claims most of this time comes from JS::Zone::getUniqueId, but that might just be inlining. I also changed GetElement to an optimized version for dense arrays, which improves the runtime by 10-20%. This was previously suggest in bug 1313373. Overall looking at some of hot assembly instructions, we should probably try to get rid of most of the Rooted on the stack. Micro optimizing IsQuoteSpecialCharacter might be a win as well, could we for example use some SSE instructions to handle more characters at once.

Comment 3

[Jan de Mooij [:jandem]]

Cycle collector time is silly, I filed bug 1342345 for that.

We can probably optimize property iteration/gets on plain objects by looking at the shapes directly instead of the slow GetPropertyKeys + GetProperty path. If we see anything weird we can fall back to the slow path. I'll file a bug for that too.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)
> Cycle collector time

Hah, s/collector/detector/

Comment 5

[Andrew Overholt [:overholt]]

Now that the dependencies are closed, is there anything left to do here, Jan?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Andrew Overholt [:overholt] from comment #5)
> Now that the dependencies are closed, is there anything left to do here, Jan?

I'm not sure but we're probably still a bit slower here due to bug 1343005 etc.

Comment 7

[(no longer active)]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Andrew Overholt [:overholt] from comment #5)
> > Now that the dependencies are closed, is there anything left to do here, Jan?
> 
> I'm not sure but we're probably still a bit slower here due to bug 1343005
> etc.

How important is the remaining work?  Should this still remain a [qf:p1]?  Can you please verify if js::Stringify is fast enough now?  Thanks!

Comment 8

[Jan de Mooij [:jandem]]

Tom do you still have the Google Drive JSON file?

Comment 9

[Tom S [:evilpie]]

I added back my logging to Stringify and I am still seeing times of 0.5s to 1s on Google Drive. I have the JSON file locally again, but I am having trouble profiling it. This JSON file is 3.4 MB, without any unnecessary whitespaces. perf just seems to shift the attribution around randomly, but Str seems to be very high.

Comment 10

[Shu-yu Guo [:shu]]

Jan suggests to remeasure against Chrome then readjust priority.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

The p1 is to remeasure this. (kannan)

Comment 12

[Kannan Vijayan [:djvj]]

Measure this again - chances are this is better now and already basically fixed.

Comment 13

[Kannan Vijayan [:djvj]]

Just measured this.  The cycle collector is not showing up at all in the profile, and by a hand-measured stopwatch load timer, we look about the same on load-time as chrome on my machine.  Given that both the bugs (bug 1343005 and bug 1342345) that Jan mentioned as issues have been fixed, and the re-measuring shows parity, I think we can mark this as fixed.