Closed Bug 1436680 Opened 4 years ago Closed 4 years ago

Allow non-templated uses of AutoTimer

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: Dexter, Assigned: o0ignition0o, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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?
Flags: needinfo?(gfritzsche)
Flags: needinfo?(chutten)
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?
Flags: needinfo?(gfritzsche)
(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
Flags: needinfo?(chutten)
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!
Assignee: nobody → jeremy.lempereur
Mentor: alessio.placitelli
Priority: -- → P1
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.
Blocks: 1438112
(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
Attachment #8950666 - Flags: review?(alessio.placitelli) → review+
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
Flags: needinfo?(jeremy.lempereur)
Oh my bad I thought his nick was jvarga ^^ fixing it right now :)
Flags: needinfo?(jeremy.lempereur)
(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
Attachment #8950665 - Flags: review?(alessio.placitelli) → review+
Mentor: chutten
:janv, can you take a look at this?
Flags: needinfo?(jvarga)
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!
Attachment #8950665 - Flags: review?(jvarga) → review+
Flags: needinfo?(jvarga)
Keywords: checkin-needed
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
Keywords: checkin-needed
Looks like C++ initializer list/member order mismatch?
Flags: needinfo?(jeremy.lempereur)
Yes indeed !
I'll be away until tuesday, is it ok if we wait until then ?
Flags: needinfo?(jeremy.lempereur)
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?
Attachment #8950666 - Flags: review?(chutten) → review+
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?
Attachment #8950665 - Flags: review?(chutten) → review+
Thanks for such a thorough review !
I'll try to address it tomorrow
Blocks: 1444735
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 ? :)
Flags: needinfo?(chutten)
It has my r+, doesn't it? :)

Yes, you may ship it.
Flags: needinfo?(chutten)
Oh yeah, sorry for being shy, it's the first time I add the checkin needed flag ^^'
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0cc01cf45269
https://hg.mozilla.org/mozilla-central/rev/5d02f576ee2a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.