1374628 - Improve static_asserts in TelemetryHistogram.cpp

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

With bug 1374419 solved, we can now do the static asserts here properly:
https://dxr.mozilla.org/mozilla-central/rev/416c3c8c4b3db9ba96a103ce7820c9a140a3051d/toolkit/components/telemetry/TelemetryHistogram.cpp#1890

We need to change the asserts to the form of:
> static_assert((JS::gcreason::NUM_TELEMETRY_REASONS + 1) == gHistograms[mozilla::Telemetry::GC_MINOR_REASON].bucketCount, "...");

We have to check JS::gcreason::NUM_TELEMETRY_REASONS against GC_MINOR_REASON, GC_MINOR_REASON_LONG, GC_REASON_2.
mozilla::StartupTimeline::MAX_EVENT_ID needs to be checked against STARTUP_MEASUREMENT_ERRORS.

We also need to update the comment.

After the changes the build should still work properly.

Comment 1

[Igor Timofeev [:ivtimofeev]]

Hello, I would like to have a go at this if possible.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Great, i assigned it to you.

Do you have a working Firefox build yet?
If not you can follow the information here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build

Comment 3

[Igor Timofeev [:ivtimofeev]]

Attachment: Improve static_asserts in TelemetryHistogram.cpp

With bug 1374419 solved, static asserts in TelemetryHistogram.cpp can be improved.

Specifically, JS::gcreason::NUM_TELEMETRY_REASONS can now be compared against GC_MINOR_REASON, GC_MINOR_REASON_LONG, and GC_REASON_2 in the now constexpr gHistograms. Likewise, mozilla::StartupTimeline::MAX_EVENT_ID can be compared against STARTUP_MEASUREMENT_ERRORS.

This commit modifies the static asserts to perform these comparisons in place of ones against hardcoded values and modifies the accompanying comment to remove the TODO section.

Comment 4

[Igor Timofeev [:ivtimofeev]]

First attempt, wasn't sure how exactly to format code to fit guidelines. Build still works after the changes.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8879952 [details] [diff] [review]
Improve static_asserts in TelemetryHistogram.cpp

Review of attachment 8879952 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good!
Only requesting some small changes below.

You can also change the commit message a little. The recommend pattern is:
> Bug xxx - ...
> <blank line>
> ... more text

That clearly separates the short message from the detailed description.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1895,5 @@
> +                        gHistograms[mozilla::Telemetry::GC_REASON_2].bucketCount,
> +                  "NUM_TELEMETRY_REASONS is assumed to be a fixed value in Histograms.json."
> +                  " If this was an intentional change, update this assert with its value "
> +                  "and update the n_values for the following in Histograms.json: "
> +                  "GC_MINOR_REASON, GC_MINOR_REASON_LONG, GC_REASON_2");

We need to update the message now and remove the part: "update this assert with its value and".

@@ +1896,5 @@
> +                  "NUM_TELEMETRY_REASONS is assumed to be a fixed value in Histograms.json."
> +                  " If this was an intentional change, update this assert with its value "
> +                  "and update the n_values for the following in Histograms.json: "
> +                  "GC_MINOR_REASON, GC_MINOR_REASON_LONG, GC_REASON_2");
> +    

Nit: There is some trailing whitespace on this line.
Follow the "review" link to see it highlighted in red.

@@ +1900,5 @@
> +    
> +    static_assert((mozilla::StartupTimeline::MAX_EVENT_ID + 1) ==
> +                        gHistograms[mozilla::Telemetry::STARTUP_MEASUREMENT_ERRORS].bucketCount,
> +                  "MAX_EVENT_ID is assumed to be a fixed value in Histograms.json.  If this"
> +                  " was an intentional change, update this assert with its value and update"

We need to update the message now and remove the part: "update this assert with its value and".

Comment 6

[Igor Timofeev [:ivtimofeev]]

Attachment: Improve static_asserts in TelemetryHistogram.cpp

With bug 1374419 solved, static asserts in TelemetryHistogram.cpp can be improved.

Specifically, JS::gcreason::NUM_TELEMETRY_REASONS can now be compared against GC_MINOR_REASON, GC_MINOR_REASON_LONG, and GC_REASON_2 in the now constexpr gHistograms. Likewise, mozilla::StartupTimeline::MAX_EVENT_ID can be compared against STARTUP_MEASUREMENT_ERRORS.

This commit modifies the static asserts to perform these comparisons in place of ones against hardcoded values and modifies the accompanying comment to remove the TODO section.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Thanks for the patch!

Comment 8

[Igor Timofeev [:ivtimofeev]]

Thank you very much for reviewing

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dedf019826a
Improve static_asserts in TelemetryHistogram.cpp. r=gfritzsche

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6dedf019826a