1369171 - Telemetry::AccumulateChild is slow when it processes lots of accumulations

Status: RESOLVED DUPLICATE of bug 1366294

(Toolkit :: Telemetry, defect, P3)

Description

[Markus Stange [:mstange]]

It does many repeated histogram lookups, and each of those lookups does string concatenation.

Steps to reproduce are in bug 1369125.

Here's a profile: https://perfht.ml/2rkW1RM

Comment 1

[Markus Stange [:mstange]]

Attachment: proof of concept patch

What do you think of this approach?

This inlines all of internal_AccumulateChild and parts of internal_HistogramAdd into TelemetryHistogram::AccumulateChild.
Profile before: https://perfht.ml/2rkW1RM
Profile after: https://perfht.ml/2rldWbl

It's not going to help if accumulations from multiple IDs are interleaved.

Alternative ways to fix this bug would be:
 - Stop doing so much string concatenation
 - Find a way to check the subsession cache without doing expensive works before the check
 - Pre-accumulate in the child?

Comment 2

[Markus Stange [:mstange]]

Attachment: alternative patch

This one makes quite a difference as well. It's an alternative to the other patch.

This adds variants of internal_HistogramAdd and internal_GetSubsessionHistogram
that take a HistogramID. Callers which already have the histogram ID can use
these variants to save some repeated lookups.

Profile before: https://perfht.ml/2rkW1RM
Profile after: https://perfht.ml/2rlyQH9

Comment 3

[Chris H-C :chutten]

Also note Ehsan's attempt at bug 1350765
Also also note :gfritzsche's storage refactor underway at bug 1366294

Comment 4

[Markus Stange [:mstange]]

(In reply to Markus Stange [:mstange] from comment #1)
>  - Find a way to check the subsession cache without doing expensive work
> before the check

The expensive work here is calling GetProcessFromName(existing.histogram_name()), which reads the string. It would be much better to have a ProcessID stored on the histogram itself.

It looks like Histogram is defined in chromium code and ProcessID is defined in Telemetry code. Maybe we should have a wrapper class that adds fields like the process ID and the regular name.

Comment 5

[Markus Stange [:mstange]]

(In reply to Chris H-C :chutten from comment #3)
> Also note Ehsan's attempt at bug 1350765

Haha, Ehsan's first patch is almost identical to my second patch.

Comment 6

[(no longer active)]

(In reply to Markus Stange [:mstange] from comment #5)
> (In reply to Chris H-C :chutten from comment #3)
> > Also note Ehsan's attempt at bug 1350765
> 
> Haha, Ehsan's first patch is almost identical to my second patch.

We should obviously wait to see if someone manages to reinvent this wheel again a third time.  :D

Comment 7

[(no longer active)]

Markus, see https://bugzilla.mozilla.org/show_bug.cgi?id=1350765#c21 if you felt like trying this again, watch out for the talos regression and histogram.cc warnings going haywire!

Comment 8

[Markus Stange [:mstange]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #7)
> Markus, see https://bugzilla.mozilla.org/show_bug.cgi?id=1350765#c21 if you
> felt like trying this again

Nah I'm good, bug 1366294 comment 0 describes exactly what I'd want here, especially the part about
>   - a O(1) table for (HistogramID, ProcessID, isSubsession) -> Histogram*

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Markus Stange [:mstange] from comment #8)
> Nah I'm good, bug 1366294 comment 0 describes exactly what I'd want here,
> especially the part about
> >   - a O(1) table for (HistogramID, ProcessID, isSubsession) -> Histogram*

Right, i think we keep hitting the problem of "the historical lookup code for histogram is bad" from multiple angles, so it seems time to clean that up properly.
Let's revisit this bug after the refactoring in bug 1366294?

Comment 10

[Georg Fritzsche [:gfritzsche]]

Ok, looking at the patches, bug 1366294 already does this work.
Thanks for the active suggestions though!