We saw GhostWindowsReporter [1] showing up in profiles [2] in bug 1375281, specifically the part calling int nsEffectiveTLDService [3] We should look into speeding this up. [1] http://searchfox.org/mozilla-central/rev/77b256dc98efb93f1d118db456f442a09bba77c2/dom/base/nsWindowMemoryReporter.h#172 [2] http://bit.ly/2t0WBYb [3] https://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/netwerk/dns/nsEffectiveTLDService.cpp#51

There are a few inefficiencies here: #1 - We're using a sorted array of 6271 elements to try to match domains, we could probably use a better structure (perhaps a trie) #2 - We iterate over subdomains until we get a match, but the largest eTLD is 4 labels (ie: foo.bar.com.eu). So if a URL has 10 labels, we'll iterate 6 times doing roughly log2(6271) ~= 13 attempts at matching for each iteration. In this example that's 78 unnecessary strcmps against strings up to 37 chars #3 - The strings in our string table are 1-byte aligned, this could possibly lead to slower comparisons #4 - The ghost window function doesn't check if a domain has already been mapped #5 - For this measurement we probably don't need to recount the number of ghost windows, we can just use the cached value #5 and #4 are probably the quickest wins, #3 is a bit nebulous, #2 would be somewhat straightforward but it could be a perf tradeoff for shorter URLs, #1 is a larger endeavor but using something like gperf [1] to generate a perfect hashtable at compile time instead might make sense. [1] http://www.gnu.org/software/gperf/

We already periodically calculate the ghost window amount after cycle collection, this just uses a cached value of that for the distinguished amount. This avoids the overhead of a recalculating the value when reporting telemetry.

Avoid hidding the rather slow effective TLD service by caching results when mapping URLs to their base domains. In testing the cache ranged from a 1:1 to a 3:1 hit:miss ratio.

This combines the GhostWindowsReporter with the nsWindowMemoryReporter. It has the benefit of removing a reporter of a single value and also guarntees that we use the latests ghost windows value that is calculated in |nsWindowMemoryReporter::CollectReports| rather than a possibly cached value from a previous run.

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #3) > Created attachment 8881901 [details] [diff] [review] > Part 2: Cache base domains during ghost window calculation > > Avoid hidding the rather slow effective TLD service by caching results when hitting?

Comment on attachment 8881902 [details] [diff] [review] Part 3: Combine ghost window reporter with window reporter Review of attachment 8881902 [details] [diff] [review]: ----------------------------------------------------------------- Nice. "guarantees" is misspelt in the commit message. ::: dom/base/nsWindowMemoryReporter.cpp @@ +491,5 @@ > aData); > } > > + MOZ_COLLECT_REPORT( > + "ghost-windows", KIND_OTHER, UNITS_COUNT, ghostWindows.Count(), This line is over-indented.

(In reply to Nicholas Nethercote [:njn] from comment #5) > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > #3) > > Created attachment 8881901 [details] [diff] [review] > > Part 2: Cache base domains during ghost window calculation > > > > Avoid hidding the rather slow effective TLD service by caching results when > > hitting? Ah, good catch!

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #1) > There are a few inefficiencies here: > > #1 - We're using a sorted array of 6271 elements to try to match domains, we > could probably use a better structure (perhaps a trie) > > #1 is a larger endeavor but using something like gperf [1] to generate a > perfect hashtable at compile time instead might make sense. > > [1] http://www.gnu.org/software/gperf/ Interestingly njn converted *from* a hashtable in bug 1247835, but I think switching back to a perfect hash is still probably the way to go. Luckily we have a perf test to confirm this.

Comment on attachment 8881900 [details] [diff] [review] Part 1: Use a cached ghost window value for the distinguished amount Review of attachment 8881900 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsWindowMemoryReporter.cpp @@ +823,5 @@ > > /* static */ int64_t > nsWindowMemoryReporter::GhostWindowsReporter::DistinguishedAmount() > { > + return sWindowReporter->mGhostWindowCount; Maybe update the description for this reporter to say that it is a recent cached value?

Comment on attachment 8881901 [details] [diff] [review] Part 2: Cache base domains during ghost window calculation Review of attachment 8881901 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsWindowMemoryReporter.cpp @@ +752,5 @@ > if (uri) { > + domain = domainMap.LookupForAdd(uri).OrInsert([&]() { > + nsCString d; > + tldService->GetBaseDomain(uri, 0, d); > + return d; So this is returning a copy of the string?

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #8) > (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment > #1) > > There are a few inefficiencies here: > > > > #1 - We're using a sorted array of 6271 elements to try to match domains, we > > could probably use a better structure (perhaps a trie) > > > > #1 is a larger endeavor but using something like gperf [1] to generate a > > perfect hashtable at compile time instead might make sense. > > > > [1] http://www.gnu.org/software/gperf/ > > Interestingly njn converted *from* a hashtable in bug 1247835, but I think > switching back to a perfect hash is still probably the way to go. Luckily we > have a perf test to confirm this. I did some tests with a perfect hash and it was ~30% faster, but added a lot of overhead memory wise. I took a look at what chromium does, it appears they switched from a perfect hash (gperf) to a dafsa (essentially an optimzed trie graph). Integrating that showed a negligable speedup but maybe 100k reduction in size which is nice. I'll spin off a bug for that as it's a bit involved to integrate it and probably isn't a big perf win.

(In reply to Andrew McCreight [:mccr8] from comment #9) > Comment on attachment 8881900 [details] [diff] [review] > Part 1: Use a cached ghost window value for the distinguished amount > > Review of attachment 8881900 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/nsWindowMemoryReporter.cpp > @@ +823,5 @@ > > > > /* static */ int64_t > > nsWindowMemoryReporter::GhostWindowsReporter::DistinguishedAmount() > > { > > + return sWindowReporter->mGhostWindowCount; > > Maybe update the description for this reporter to say that it is a recent > cached value? Will update.

(In reply to Andrew McCreight [:mccr8] from comment #10) > Comment on attachment 8881901 [details] [diff] [review] > Part 2: Cache base domains during ghost window calculation > > Review of attachment 8881901 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/nsWindowMemoryReporter.cpp > @@ +752,5 @@ > > if (uri) { > > + domain = domainMap.LookupForAdd(uri).OrInsert([&]() { > > + nsCString d; > > + tldService->GetBaseDomain(uri, 0, d); > > + return d; > > So this is returning a copy of the string? The lambda returns a copy, |OrInsert| will execute the lambda and store the result if it's not already in the table, that returns a ref.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c99b4e8e6c1c2046b71bb7128444f380a967fd1 Bug 1376038 - Part 1: Use a cached ghost window value for the distinguished amount. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/ae6472c175774fa381757f13ea193e4244649fb8 Bug 1376038 - Part 2: Cache base domains during ghost window calculation. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/b89df6ea668a9e692201f9a7e740508263d4fa17 Bug 1376038 - Part 3: Combine ghost window reporter with window reporter. r=njn

https://hg.mozilla.org/mozilla-central/rev/8c99b4e8e6c1 https://hg.mozilla.org/mozilla-central/rev/ae6472c17577 https://hg.mozilla.org/mozilla-central/rev/b89df6ea668a

Sheriffs, please backout the patches in this bug (and in bug 1384337, which landed on top of this) from Nightly. I want to see if the lazier recording of telemetry is what caused bug 1388111.

Backed out from mozilla-central along with bug 1384337 to help with the investigation of bug 1388111. https://hg.mozilla.org/integration/mozilla-inbound/rev/aae99e32153ff4de35f1e996ababc7c757bf8f49 Note that the status is a little screwy here because Beta56 still has the patches in question.

also backout landed on m-c https://hg.mozilla.org/mozilla-central/rev/aae99e32153f

https://hg.mozilla.org/integration/mozilla-inbound/rev/366cb8c89311c297ea837387bb621a1c30da8e3d Bug 1376038 - Part 1: Use a cached ghost window value for the distinguished amount. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/1341377398fe331eccff1305c5ccd4a1a0b52012 Bug 1376038 - Part 2: Cache base domains during ghost window calculation. r=mccr8 https://hg.mozilla.org/integration/mozilla-inbound/rev/dce4d03cf87aef8740af78872e8822e817af3dc6 Bug 1376038 - Part 3: Combine ghost window reporter with window reporter. r=njn

(In reply to (Out 9/7 - 9/8) Eric Rahm [:erahm] (please no mozreview requests) from comment #19) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > 366cb8c89311c297ea837387bb621a1c30da8e3d > Bug 1376038 - Part 1: Use a cached ghost window value for the distinguished > amount. r=mccr8 Note: this includes a rollup of 1384337.

https://hg.mozilla.org/mozilla-central/rev/366cb8c89311 https://hg.mozilla.org/mozilla-central/rev/1341377398fe https://hg.mozilla.org/mozilla-central/rev/dce4d03cf87a