At least one consumer [1] re-implemented |Telemetry::AutoTimer| because it required the histogram id to be fed at run-time, not compilation time. I think that's a shortcoming of our current API: we should probably provide some similar functionality out of the box. [1] - https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/dom/storage/LocalStorageCache.cpp#256-272

Georg, Chris - what's your take on this?

I doubt that template-parameterizing on the Histogram ID actually makes any measurable difference. How about we just move the histogram id passing to a constructor parameter for all code?

(In reply to Georg Fritzsche [:gfritzsche] from comment #2) > I doubt that template-parameterizing on the Histogram ID actually makes any > measurable difference. > > How about we just move the histogram id passing to a constructor parameter > for all code? This sounds good to me. I'll set this up as a C++ mentored.

AutoCounter uses the same approach, if we change one let's change both to stay consistent (either here or in a follow-up).

Looking at the users of AutoTimer[1] there are some potential hot paths that could be affected (nsThread stands out as a potential). No doubt talos will catch us if we do anything _too_ egregious, but perhaps we should stick to adding a runtime-specifiable edition of autotimer, leaving the old one alone. [1]: https://searchfox.org/mozilla-central/search?q=symbol:T_mozilla%3A%3ATelemetry%3A%3AAutoTimer&redirect=false

Can I take this one once I'm done with Bug 1432791 ?

(In reply to Chris H-C :chutten from comment #5) > Looking at the users of AutoTimer[1] there are some potential hot paths that > could be affected (nsThread stands out as a potential). No doubt talos will > catch us if we do anything _too_ egregious, but perhaps we should stick to > adding a runtime-specifiable edition of autotimer, leaving the old one alone. > > [1]: > https://searchfox.org/mozilla-central/search?q=symbol: > T_mozilla%3A%3ATelemetry%3A%3AAutoTimer&redirect=false Fair enough. One approach we could have would be to add the non templated version in this bug, then file a follow-up for evaluating changing the templated version to the newly added one. There we could simply trigger a talos build on try to decide :) (In reply to Jeremy Lempereur from comment #6) > Can I take this one once I'm done with Bug 1432791 ? Sure! That would be great!

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review225948 This looks good with the changes below addressed :) Nothing major! ::: dom/storage/LocalStorageCache.cpp (Diff revision 1) > -namespace { > - > -// The AutoTimer provided by telemetry headers is only using static, > -// i.e. compile time known ID, but here we know the ID only at run time. > -// Hence a new class. > -class TelemetryAutoTimer This will require a review from the owner of that module. Let's flag :mak there. ::: toolkit/components/telemetry/Telemetry.h:244 (Diff revision 1) > +class MOZ_RAII RuntimeAutoTimer > +{ > +public: > + explicit RuntimeAutoTimer(Telemetry::HistogramID aId, > + TimeStamp aStart = TimeStamp::Now() > + MOZ_GUARD_OBJECT_NOTIFIER_PARAM) nit: let's align `MOZ_GUARD_OBJECT_NOTIFIER_PARAM` to the `TimeStamp` on the previous line (i.e. two whitespaces to the left) ::: toolkit/components/telemetry/Telemetry.h:253 (Diff revision 1) > + MOZ_GUARD_OBJECT_NOTIFIER_INIT; > + } > + explicit RuntimeAutoTimer(Telemetry::HistogramID aId, > + const nsCString& aKey, > + TimeStamp aStart = TimeStamp::Now() > + MOZ_GUARD_OBJECT_NOTIFIER_PARAM) nit: let's align `MOZ_GUARD_OBJECT_NOTIFIER_PARAM` to the `TimeStamp` on the previous line (i.e. two whitespaces to the left) ::: toolkit/components/telemetry/docs/collection/histograms.rst:330 (Diff revision 1) > +If the HistogramID is not known at compile time, one can use the ``RuntimeAutoTimer`` class, which behaves like the template parameterized ``AutoTimer``. > + > +.. code-block:: cpp > + > + void > + LocalStorageCache::WaitForPreload(Telemetry::HistogramID aTelemetryID) Let's be generic here, without pointing to a specific class ;) Just change the name of the function to something like `FunctionWithTiming` and drop the reference to the `LocalStorageCache`.

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review225954 ::: toolkit/components/telemetry/Telemetry.h:238 (Diff revision 1) > void SetHistogramRecordingEnabled(HistogramID id, bool enabled); > > const char* GetHistogramName(HistogramID id); > > +// Prefer using the AutoTimer class > +// if you know the HistogramID at compile time. Let's drop this comment: let's call out in the docs that the templated version is preferred on hot paths, if possible.

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review225958 ::: toolkit/components/telemetry/docs/collection/histograms.rst:325 (Diff revision 1) > Telemetry::AutoTimer<Telemetry::PLUGIN_SHUTDOWN_MS> timer; > ... > return NS_OK; > } > + > +If the HistogramID is not known at compile time, one can use the ``RuntimeAutoTimer`` class, which behaves like the template parameterized ``AutoTimer``. Can you please also update the [`measuring-time`](https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/collection/measuring-time.rst) doc?

Comment on attachment 8950666 [details] Bug 1436680 - Allow non-templated uses of AutoCounter. https://reviewboard.mozilla.org/r/219918/#review225956 ::: toolkit/components/telemetry/Telemetry.h:310 (Diff revision 1) > const TimeStamp start; > const nsCString key; > MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER > }; > > +// Prefer using the AutoCounter class Let's drop this and call out in the docs that on hot paths we prefer the templated version. ::: toolkit/components/telemetry/docs/collection/histograms.rst:339 (Diff revision 1) > Telemetry::RuntimeAutoTimer timer(aTelemetryID); > ... > } > + > + int32_t > + Predictor::CalculateConfidence(uint32_t hitCount, uint32_t hitsPossible, Let's just provide a sample function name here as well.

(In reply to Alessio Placitelli [:Dexter] from comment #10) > Comment on attachment 8950665 [details] > Bug 1436680 - Allow non-templated uses of AutoTimer. > > https://reviewboard.mozilla.org/r/219850/#review225948 > > This looks good with the changes below addressed :) Nothing major! > > ::: dom/storage/LocalStorageCache.cpp > (Diff revision 1) > > -namespace { > > - > > -// The AutoTimer provided by telemetry headers is only using static, > > -// i.e. compile time known ID, but here we know the ID only at run time. > > -// Hence a new class. > > -class TelemetryAutoTimer > > This will require a review from the owner of that module. Let's flag :mak > there. Mh, maybe jvarga instead of :mak!

Comment on attachment 8950666 [details] Bug 1436680 - Allow non-templated uses of AutoCounter. https://reviewboard.mozilla.org/r/219918/#review226382

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review226384 ::: toolkit/components/telemetry/docs/collection/measuring-time.rst:91 (Diff revision 2) > > + // If the Histogram id is not known at compile time: > + class RuntimeAutoTimer { > + // Record into a plain histogram. > + explicit RuntimeAutoTimer(Telemetry::HistogramID aId, > + TimeStamp aStart = TimeStamp::Now()) nit: it's missing the trailing `;` (we have that above) ::: toolkit/components/telemetry/docs/collection/measuring-time.rst:93 (Diff revision 2) > + class RuntimeAutoTimer { > + // Record into a plain histogram. > + explicit RuntimeAutoTimer(Telemetry::HistogramID aId, > + TimeStamp aStart = TimeStamp::Now()) > + // Record into a keyed histogram, with key |aKey|. > + explicit RuntimeAutoTimer(Telemetry::HistogramID aId, nit: it's missing the trailing `;` ::: toolkit/components/telemetry/docs/collection/measuring-time.rst:96 (Diff revision 2) > + TimeStamp aStart = TimeStamp::Now()) > + // Record into a keyed histogram, with key |aKey|. > + explicit RuntimeAutoTimer(Telemetry::HistogramID aId, > + const nsCString& aKey, > + TimeStamp aStart = TimeStamp::Now()) > + nit: Let's add a closing `};` at the end of the class snippet :) ::: toolkit/components/telemetry/docs/collection/measuring-time.rst (Diff revision 2) > void AccumulateTimeDelta(HistogramID id, const nsCString& key, TimeStamp start, TimeStamp end = TimeStamp::Now()); > > Example: > > .. code-block:: cpp > - Don't remove this blank line otherwise the code block will fail to render in the generated docs.

Hey Jeremy! Can you please add Jan Varga for reviewing the local storage cache changes? His handle is janv. Just change the commit message to r?dexter,janv

Oh my bad I thought his nick was jvarga ^^ fixing it right now :)

(In reply to Jeremy Lempereur from comment #20) > Oh my bad I thought his nick was jvarga ^^ fixing it right now :) Not your fault, my fault :) I thought it was jvarga as well, but I double checked and it's janv :)

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review226390

:janv, can you take a look at this?

If you would like me to change the commit message so someone else can review it, please let me know :)

Yes, I'll take a look.

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review229558 Looks good, thanks!

Pushed by btara@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4258b9e2ebfc Allow non-templated uses of AutoTimer. r=Dexter,janv https://hg.mozilla.org/integration/autoland/rev/66fb8813478c Allow non-templated uses of AutoCounter. r=Dexter

Backed out 2 changesets (bug 1436680) for bustages on /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Telemetry.h. a=backout on a CLOSED TREE push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=66fb8813478c57245f4149af64c2397c03ad028f bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=109cd0a34ffea65f7540dbd6d7feb9c6ca39ec55&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=164771959&filter-searchStr=Linux%20opt%20build-linux%2Fopt%20(B) https://treeherder.mozilla.org/logviewer.html#?job_id=164771959&repo=autoland backout: https://hg.mozilla.org/integration/autoland/rev/05aaf7f46525408a1294b9ec313458faa5b64213

Looks like C++ initializer list/member order mismatch?

Yes indeed ! I'll be away until tuesday, is it ok if we wait until then ?

Ok I think this should work. :chutten two questions remain : 1 - Is there a way for me to setup my local build the way the CI is set up ? (-Wreorder flags are just warnings so I can't be sure I'm done with it) 2 - Should I wait for dexter and janv to approve my PR again before I add the check-in flag? Thanks a lot !

It is theoretically possible to set yourself up more-or-less like CI, but the big one you might be missing is to put ac_add_options --enable-warnings-as-errors in your mozconfig. When you have things sorted out, flag me instead of Dexter on the new patch (he's out for a few weeks)

(Also, we can push this direct to CI using the tryserver and see if there's anything else that crops up)

I've been able to reproduce the build error with the option you suggested, and I've been able to build with the new patch I've submitted. Would you like me to flag you or should we run a try instead ?

Comment on attachment 8950666 [details] Bug 1436680 - Allow non-templated uses of AutoCounter. https://reviewboard.mozilla.org/r/219918/#review231646 A few small things, but overall looks okay. ::: toolkit/components/telemetry/Telemetry.h:334 (Diff revision 6) > + } > + > + // Chaining doesn't make any sense, don't return anything. > + void operator+=(int increment) > + { > + counter += increment; Please file a follow-up bug for guarding these operations (both templated and runtime) against overflow/underflow. ::: toolkit/components/telemetry/docs/collection/histograms.rst:342 (Diff revision 6) > > + int32_t > + FunctionWithCounter(Telemetry::HistogramID aTelemetryID) > + { > + ... > + // Increment an AutoCounter We can omit the comment ::: toolkit/components/telemetry/docs/collection/histograms.rst:344 (Diff revision 6) > + FunctionWithCounter(Telemetry::HistogramID aTelemetryID) > + { > + ... > + // Increment an AutoCounter > + Telemetry::RuntimeAutoCounter myCounter(aTelemetryID); > + ++myCounter; Include a use of operator+= as well?

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review231642 Fix these small things and you'll be good to go. ::: toolkit/components/telemetry/docs/collection/histograms.rst:333 (Diff revision 6) > + > + void > + FunctionWithTiming(Telemetry::HistogramID aTelemetryID) > + { > + ... > + // Measure which operation blocks and for how long I think we can omit the comment ::: toolkit/components/telemetry/docs/collection/measuring-time.rst:99 (Diff revision 6) > + const nsCString& aKey, > + TimeStamp aStart = TimeStamp::Now()); > + > void AccumulateTimeDelta(HistogramID id, TimeStamp start, TimeStamp end = TimeStamp::Now()); > void AccumulateTimeDelta(HistogramID id, const nsCString& key, TimeStamp start, TimeStamp end = TimeStamp::Now()); > + }; Uh, AccumulateTimeDelta isn't part of the RuntimeAutoTimer class, is it?

Thanks for such a thorough review ! I'll try to address it tomorrow

Comment on attachment 8950665 [details] Bug 1436680 - Allow non-templated uses of AutoTimer. https://reviewboard.mozilla.org/r/219850/#review231642 > Uh, AccumulateTimeDelta isn't part of the RuntimeAutoTimer class, is it? Indeed, good catch ! (it used to be outside the class)

Can I ship it ? :)

It has my r+, doesn't it? :) Yes, you may ship it.

Oh yeah, sorry for being shy, it's the first time I add the checkin needed flag ^^'

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0cc01cf45269 Allow non-templated uses of AutoTimer. r=chutten,Dexter,janv https://hg.mozilla.org/integration/autoland/rev/5d02f576ee2a Allow non-templated uses of AutoCounter. r=chutten,Dexter

https://hg.mozilla.org/mozilla-central/rev/0cc01cf45269 https://hg.mozilla.org/mozilla-central/rev/5d02f576ee2a