Closed Bug 1374628 Opened 4 years ago Closed 4 years ago

Improve static_asserts in TelemetryHistogram.cpp

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement
Points:
1

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: gfritzsche, Assigned: ivtimofeev, Mentored)

References

Details

(Whiteboard: [measurement:client] [lang=c++] [good first bug])

Attachments

(1 file, 1 obsolete file)

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.
Points: --- → 1
Hello, I would like to have a go at this if possible.
Flags: needinfo?(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
Assignee: nobody → igor.timofeev
Flags: needinfo?(gfritzsche)
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.
First attempt, wasn't sure how exactly to format code to fit guidelines. Build still works after the changes.
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".
Attachment #8879952 - Flags: feedback+
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.
Attachment #8879972 - Flags: review+
Thanks for the patch!
Attachment #8879952 - Attachment is obsolete: true
Keywords: checkin-needed
Thank you very much for reviewing
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dedf019826a
Improve static_asserts in TelemetryHistogram.cpp. r=gfritzsche
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6dedf019826a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.