1851874 - Too much minorGC time in Charts-observable-plot slice calls

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, task)

Description

[Markus Stange [:mstange]]

https://shell--speedometer-preview.netlify.app/?suite=Charts-observable-plot

The function token on the "Charts-observable-plot" Speedometer 3 subtest calls slice to create many small strings.

We spend 1.24x as much time as Chrome in this function, and most of that difference is spent in minorGC.

Firefox: https://share.firefox.dev/3sOpWFL 3034 samples (597 samples in GCRuntime::minorGC)
Chrome: https://share.firefox.dev/3sPyb4r 2436 samples (140 samples in Heap::Scavenge)

Making Firefox as fast as Chrome on this function (including GC time in this function) would improve Firefox's score on this subtest by 1.3%.

The split call on Editor-CodeMirror (see bug 1844868) shows something similar.

Comment 1

[Iain Ireland [:iain]]

I see 461 samples inside Nursery::doCollection, of which 264 (57%) are in TenuringTracer::onStringEdge. Of those, at least 215 are spent deduplicating strings (38 in HashSet::add and 177 in HashSet::lookupForAdd).

Steve, you know things about string deduplication, right? Any thoughts about improving it here? Is there an easy way to turn it off and measure how that affects Speedometer performance?

Comment 2

[Iain Ireland [:iain]]

Thinking about this a little more: would it make sense to decide whether or not to deduplicate strings based on the GC reason? If we are doing a collection during idle time, deduplication seems like a clear win. If we're doing it because we ran out of nursery space, then it's more of a tradeoff.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Iain Ireland [:iain] from comment #1)

Steve, you know things about string deduplication, right? Any thoughts about improving it here? Is there an easy way to turn it off and measure how that affects Speedometer performance?

There's some data in bug 1846297.

Comment 4

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

The many small strings created by slice are dependent strings, which must be scanned and handled specially with deduplication. I did an experiment where js::NewDependentString creates an inline string instead if the requested string would fit into a thin inline string. (I did not allow it to use fat inline strings, since that would increase memory usage, but perhaps I should do that experiment as well.) I saw an overall speedup of 1.7% for Charts-observable-plot (based on 10 before-patch samples and 99 after-patch samples).

I haven't checked to see what the speedup is from. If it doesn't regress anything, it's probably a good change regardless of whether it speeds up deduplication itself. I'll do a try push to check.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)

The many small strings created by slice are dependent strings, which must be scanned and handled specially with deduplication. I did an experiment where js::NewDependentString creates an inline string instead if the requested string would fit into a thin inline string. (I did not allow it to use fat inline strings, since that would increase memory usage, but perhaps I should do that experiment as well.) I saw an overall speedup of 1.7% for Charts-observable-plot (based on 10 before-patch samples and 99 after-patch samples).

We do have some code for that in JSDependentString::new_: https://searchfox.org/mozilla-central/rev/2f5b657343ce18e4b8a56417f707022550f4b742/js/src/vm/StringType-inl.h#227-236

Comment 6

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

Doh! Sure enough, I just implemented something that was already there. No wonder it didn't make much of a difference. These method names lie.

Comment 7

[Iain Ireland [:iain]]

Jan's change in bug 1898280 to prevent chains of dependent substrings helped with a number of cases where we spent a long time deduplicating strings. It might have had a similar effect here. I no longer see a lot of minorGC time in charts-observable, and in fact it seems like Chrome might spend more time in GC in this subtest than we do.