Improve static_asserts in TelemetryHistogram.cpp

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: gfritzsche, Assigned: ivtimofeev, Mentored)

Tracking

Trunk
mozilla56
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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.
Reporter

Updated

2 years ago
Points: --- → 1
Assignee

Comment 1

2 years ago
Hello, I would like to have a go at this if possible.
Assignee

Updated

2 years ago
Flags: needinfo?(gfritzsche)
Reporter

Comment 2

2 years ago
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)
Assignee

Comment 3

2 years ago
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.
Assignee

Comment 4

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

Comment 5

2 years ago
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+
Assignee

Comment 6

2 years ago
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.
Reporter

Updated

2 years ago
Attachment #8879972 - Flags: review+
Reporter

Comment 7

2 years ago
Thanks for the patch!
Reporter

Updated

2 years ago
Attachment #8879952 - Attachment is obsolete: true
Reporter

Updated

2 years ago
Keywords: checkin-needed
Assignee

Comment 8

2 years ago
Thank you very much for reviewing

Comment 9

2 years ago
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

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6dedf019826a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.