Closed Bug 1376038 Opened 7 years ago Closed 7 years ago

GhostWindowsReporter is slow

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(3 files)

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/
Assignee: nobody → erahm
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.
Attachment #8881900 - Flags: review?(continuation)
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.
Attachment #8881901 - Flags: review?(continuation)
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.
Attachment #8881902 - Flags: review?(n.nethercote)
(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.
Attachment #8881902 - Flags: review?(n.nethercote) → review+
(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?
Attachment #8881900 - Flags: review?(continuation) → review+
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?
Attachment #8881901 - Flags: review?(continuation) → review+
(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
Blocks: 1380154
Depends on: 1384337
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.
Keywords: checkin-needed
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.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
Target Milestone: mozilla56 → ---
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.