Closed Bug 1432791 Opened 6 years ago Closed 6 years ago

Can we remove the Microsecond AutoTimer resolution?

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: chutten, Assigned: o0ignition0o, Mentored)

References

Details

(Whiteboard: [lang=c++][good-next-bug])

Attachments

(1 file)

Bug 1432791 - Remove the Microsecond AutoTimer resolution.
59 bytes, text/x-review-board-request
Dexter
: review+
Details 
Telemetry's AutoTimer supports two resolutions: Millisecond and Microsecond. The latter was introduced in bug 782459, requiring a little fiddling to get around Visual Studio's compiler and is currently unused.

Since it's unused, should we remove the code and specialize the AutoTimer back to Millisecond?

Or, since we've moved compilers a few times in the past five years, can we at least remove the fiddling required to satisfy VS?
Moving ni?gfritzsche from bug 1431868.
Flags: needinfo?(gfritzsche)
Paul, is the Microsecond Telemetry AutoTimer something you'd expect to be used in the near future?
Or can we remove this?
Flags: needinfo?(gfritzsche) → needinfo?(padenot)
I think that jya was going to use it?
Flags: needinfo?(padenot) → needinfo?(jyavenard)
I don't need the need for such accuracy with telemetry . While I do report microseconds, I make no use of the telemetry timers, I didn't know they existed to start with.
Flags: needinfo?(jyavenard)
Setup as mentored
Flags: needinfo?(alessio.placitelli)
Priority: -- → P3
This bug is about removing the Microsecond resolution from the |AutoTImer| class, since nobody is using it. The timer code can be found at [1]. Once the microsecond specialization is removed, the |TimerResolution| enum can be removed as well. To test that this is working, the following tests can be run:

> ./mach test toolkit/components/telemetry/tests/unit
> ./mach gtest Telemetry*

[1] - https://searchfox.org/mozilla-central/rev/e06af9c36a73a27864302cd2f829e6200dee8541/toolkit/components/telemetry/Telemetry.h#257-274,277
Mentor: alessio.placitelli, chutten
Flags: needinfo?(alessio.placitelli)
Whiteboard: [lang=c++][good-next-bug]
Hey there, mind if I try to tackle this one ? :)
(In reply to Jeremy Lempereur from comment #7)
> Hey there, mind if I try to tackle this one ? :)

Of course I don't! Please be my guest ;) Let me know if anything is unclear!
Assignee: nobody → jeremy.lempereur
Priority: P3 → P1
Comment on attachment 8949144 [details]
Bug 1432791 - Remove the Microsecond AutoTimer resolution.

https://reviewboard.mozilla.org/r/218542/#review224452

That's a good first take on this, thank you! I think we can go ahead and completely drop the `AccumulateDelta_impl`, if we add a new API endpoint. That would *really* make things simpler :) We also need to update the docs [here](https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/docs/collection/measuring-time.rst): remove the references to the resolution in the C++ code and add the new API function!

::: commit-message-4fe6f:1
(Diff revision 2)
> +Bug 1432791 - Telemetry : Remove the Microsecond AutoTimer resolution. r?dexter

Let's remove the "Telemetry : " part after the bug number

::: dom/storage/LocalStorageCache.cpp:266
(Diff revision 2)
>      : id(aId), start(TimeStamp::Now())
>    {}
>  
>    ~TelemetryAutoTimer()
>    {
> -    Telemetry::AccumulateDelta_impl<Telemetry::Millisecond>::compute(id, start);
> +    Telemetry::AccumulateDelta::compute(id, start);

Interesting, they were implementing their own AutoTimer because ours requires templates. I filed bug 1436680 to fix this.

However, they were also using something that was not really meant to be exported, `AccumulateDelta_Impl`. Let's use [`AccumulateTimeDelta`](https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/toolkit/components/telemetry/Telemetry.h#213) instead.

::: toolkit/components/reputationservice/LoginReputation.cpp:425
(Diff revision 2)
>        MOZ_ASSERT(aResolveValue == nsILoginReputationVerdictType::SAFE);
>  
>        LR_LOG(("Query login whitelist [request = %p, result = SAFE]",
>                aRequest));
>  
> -      AccumulateDelta_impl<Millisecond>::compute(
> +      AccumulateDelta::compute(

Since we're changing this, let's use `AccumulateTimeDelta` here as well.

::: toolkit/components/reputationservice/LoginReputation.cpp:448
(Diff revision 2)
>  
>          // Don't record the lookup time when there is an error, only record the
>          // result here.
>          Accumulate(LOGIN_REPUTATION_LOGIN_WHITELIST_RESULT, 2); // 2 is error
>        } else {
> -        AccumulateDelta_impl<Millisecond>::compute(
> +        AccumulateDelta::compute(

`AccumulateTimeDelta` here as well

::: toolkit/components/telemetry/Telemetry.h:222
(Diff revision 2)
>   */
>  void SetHistogramRecordingEnabled(HistogramID id, bool enabled);
>  
>  const char* GetHistogramName(HistogramID id);
>  
> -/**
> +struct AccumulateDelta

We can drop this if we use `AccumulateTimeDelta`. See my other comments

::: toolkit/components/telemetry/Telemetry.h:225
(Diff revision 2)
>      if (start > end) {
>        Accumulate(id, 0);
>        return;
>      }
>      Accumulate(id, static_cast<uint32_t>((end - start).ToMilliseconds()));

If we move this logic to the exposed API, then we can completely get rid of this class. Let's move this logic to the [`AccumulateTimeDelta`](https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/toolkit/components/telemetry/Telemetry.cpp#1990) implementation.

::: toolkit/components/telemetry/Telemetry.h:232
(Diff revision 2)
>      if (start > end) {
>        Accumulate(id, key, 0);
>        return;
>      }
>      Accumulate(id, key, static_cast<uint32_t>((end - start).ToMilliseconds()));

Let's add an `AccumulateTimeDelta` that allows to specify a key, so that we can get rid of this class. The signature should be similar to [this](https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/toolkit/components/telemetry/Telemetry.h#235). Let's add the declaration in this file, right after the existing `AccumulateTimeDelta` (please also add docstrings similar to that function). The implementation must go in the Telemetry.cpp file right below [this point](https://searchfox.org/mozilla-central/rev/a539f8f7fe288d0b4d8d6a32510be20f52bb7a0f/toolkit/components/telemetry/Telemetry.cpp#1988-1992). Let's make sure to port over the normalization logic in that function :)

::: toolkit/components/telemetry/Telemetry.h:259
(Diff revision 2)
>      MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>    }
>  
>    ~AutoTimer() {
>      if (key.IsEmpty()) {
> -      AccumulateDelta_impl<res>::compute(id, start);
> +      AccumulateDelta::compute(id, start);

Let's use `AccumulateTimeDelta` here.

::: toolkit/components/telemetry/Telemetry.h:261
(Diff revision 2)
>  
>    ~AutoTimer() {
>      if (key.IsEmpty()) {
> -      AccumulateDelta_impl<res>::compute(id, start);
> +      AccumulateDelta::compute(id, start);
>      } else {
> -      AccumulateDelta_impl<res>::compute(id, key, start);
> +      AccumulateDelta::compute(id, key, start);

Let's use `AccumulateTimeDelta` (the variation that accepts a key) here as well.

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp:1881
(Diff revision 2)
>  
>        nsCOMPtr<nsIRunnable> cbRunnable = NS_NewRunnableFunction(
>          "nsUrlClassifierDBService::AsyncClassifyLocalWithTables",
>          [callback, matchedLists, startTime]() -> void {
>            // Measure the time diff between calling and callback.
> -          AccumulateDelta_impl<Millisecond>::compute(
> +          AccumulateDelta::compute(

Same here: `AccumulateTimeDelta`
Attachment #8949144 - Flags: review?(alessio.placitelli) → review-
Wow thanks a lot for such a thorough review ! I'll come back to you asap with my next attempt !
(In reply to Jeremy Lempereur from comment #12)
> Wow thanks a lot for such a thorough review ! I'll come back to you asap
> with my next attempt !

Don't mention it, my pleasure! The patch itself it's in a good state, we just need to adjust it a little bit ;)
My last review request is still a WIP, I shouldn't have published it yet :s
Ok so I get a weird error when I try to run ./mach clang-format : 

` clang-format-5.0: /usr/lib/libtinfo.so.5: no version information available ` 

Modulo that, I think my changes are reviewable :)
Comment on attachment 8949144 [details]
Bug 1432791 - Remove the Microsecond AutoTimer resolution.

https://reviewboard.mozilla.org/r/218542/#review225126

Hey Jeremy, thank you very much for this! It looks good with the two changes below, it should be a solid r+ once the new patch comes in :) Great update to the docs, btw!

::: dom/storage/LocalStorageCache.cpp:264
(Diff revision 5)
>  public:
>    explicit TelemetryAutoTimer(Telemetry::HistogramID aId)
>      : id(aId), start(TimeStamp::Now())
>    {}
>  
> -  ~TelemetryAutoTimer()
> +  ~TelemetryAutoTimer() { Telemetry::AccumulateTimeDelta(id, start); }

nit: let's preserve the formatting of this line, no need to change it :)

::: toolkit/components/telemetry/Telemetry.h:229
(Diff revision 5)
> +
> +/**
>   * Enable/disable recording for this histogram in this process at runtime.
> - * Recording is enabled by default, unless listed at kRecordingInitiallyDisabledIDs[].
> - * id must be a valid telemetry enum, otherwise an assertion is triggered.
> + * Recording is enabled by default, unless listed at
> + * kRecordingInitiallyDisabledIDs[]. id must be a valid telemetry enum,
> + * otherwise an assertion is triggered.

Let's not change this comment: it's not relevant to this bug.
Attachment #8949144 - Flags: review?(alessio.placitelli) → review-
Comment on attachment 8949144 [details]
Bug 1432791 - Remove the Microsecond AutoTimer resolution.

https://reviewboard.mozilla.org/r/218542/#review225158
Attachment #8949144 - Flags: review?(alessio.placitelli) → review+
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/98d71952c33a
Remove the Microsecond AutoTimer resolution. r=Dexter
https://hg.mozilla.org/mozilla-central/rev/98d71952c33a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: