785931 - Remove unused probes GRADIENT_DURATION and GRADIENT_RETENTION_TIME

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Paul Adenot (:padenot)]

Bug 761393 added two telemetry probes to track the gradient retention time and the gradient generation speed.

After some time, we should get the data and choose sensible values for those two parameters.

Comment 1

[Paul Adenot (:padenot)]

Taras, where could I find the relevant data to fix this?

Comment 2

[Paul Adenot (:padenot)]

I don't plan to work on this in the near future.

Comment 3

[Chris H-C :chutten]

Since this data wasn't looked at in over four years, let's remove the two probes.

To successfully help out with this probe:

0) Comment on this bug volunteering to work on this. I will assign the bug to you and you can begin work!
1) Remove GRADIENT_DURATION and GRADIENT_RETENTION_TIME probes from toolkit/components/telemetry/Histograms.json as well as histogram-whitelists.json and where they are implemented over in layout/painting/nsCSSRenderingGradients.cpp and gfx/thebes/gfxGradientCache.cpp
2) Either submit your patch to mozreview or attach it to this bug, with me as the reviewer. I'll review the patch and submit it to our build farm to see if it introduced any build or test failures.
3) Once reviewed and tested, we'll get this landed!

If at any time you have any questions for me, you can find me on irc.mozilla.org on channel #telemetry, or you can leave a question in a comment on this bug and set the "Need more information" flag (ni?) for me.

Comment 4

[Swapnil P (:DarthSwap)]

Hi, I would like to work on this bug.

Comment 5

[Chris H-C :chutten]

Excellent! I have assigned the bug to you. Please let me know if you have any questions.

Comment 6

[Swapnil P (:DarthSwap)]

I am new and if I understand correctly, all I need to do is remove all occurrences of GRADIENT_DURATION and GRADIENT_RETENTION_TIME in the specified files, while also removing any references to these. Is there anything more that should be done?

Comment 7

[Chris H-C :chutten]

There may also be a little bit of code supporting their use in the .cpps that will be able to be removed as well, but yes. You understand it correctly.

Comment 8

[Swapnil P (:DarthSwap)]

Attachment: Bug785931.patch -Remove probes GRADIENT_DURATION, GRADIENT_RETENTION_TIME. r=chutten

I have removed GRADIENT_DURATION and GRADIENT_RETENTION_TIME probes references from all the files mentioned and have also removed the code supporting their use in the necessary .cpp files.

Comment 9

[Chris H-C :chutten]

Comment on attachment 8935824 [details] [diff] [review]
Bug785931.patch -Remove probes GRADIENT_DURATION, GRADIENT_RETENTION_TIME. r=chutten

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

The patch looks direct and clear, thank you!

The file you attached appears to be just the diff, though, and not a complete commit. We need the complete commit information to include things like the bug number, your authorship information, and a commit message (see here for suggested format and other considerations: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities )

When you have done so, please place the commit up for review the same way you did with the diff. You can put "r=chutten" in the commit message, but put r? in the review flag so I'm sure to see your review request.

Comment 10

[Swapnil P (:DarthSwap)]

Attachment: Bug785931.patch -Remove probes GRADIENT_DURATION, GRADIENT_RETENTION_TIME. r=chutten

I have removed GRADIENT_DURATION and GRADIENT_RETENTION_TIME probes references from all the files mentioned and have also removed the code supporting their use in the necessary .cpp files.

Comment 11

[Swapnil P (:DarthSwap)]

Hi! I am just following up on the patch review as I have not gotten any response. Thank You.

Comment 12

[Chris H-C :chutten]

Comment on attachment 8936000 [details] [diff] [review]
Bug785931.patch -Remove probes GRADIENT_DURATION, GRADIENT_RETENTION_TIME. r=chutten

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

Sorry for the delay, I've been at an All Hands in Austin[1] and it's been.. busy. Thank you for your patience!

Two small additions, and then we'll be good to go!

[1]: https://wiki.mozilla.org/All_Hands/Austin

::: gfx/thebes/gfxGradientCache.cpp
@@ +127,3 @@
>                                                   SystemGroup::EventTargetFor(TaskCategory::Other))
>      {
>        srand(time(nullptr));

This was the only use of Telemetry in this file, so the #include for Telemetry.h can also be removed.

::: layout/painting/nsCSSRenderingGradients.cpp
@@ +661,3 @@
>                               float aOpacity)
>  {
>    AUTO_PROFILER_LABEL("nsCSSGradientRenderer::Paint", GRAPHICS);

This was the only use of Telemetry in this file, so the #include for Telemetry.h can also be removed.

Comment 13

[Swapnil P (:DarthSwap)]

Attachment: Bug785931.patch

Bug 785931 - Removed probes GRADIENT_DURATION, GRADIENT_RETENTION_TIME as well as the Telemetry header files from .cpp files where they were not in use.

Comment 14

[Chris H-C :chutten]

Comment on attachment 8936772 [details] [diff] [review]
Bug785931.patch

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

Looks good, and builds locally without incident.

Comment 15

[Chris H-C :chutten]

Thank you for your contribution! I have marked this for inclusion in the next build.

Would you like any help finding something new to work on?

Comment 16

[Swapnil P (:DarthSwap)]

(In reply to Chris H-C :chutten from comment #15)
> Thank you for your contribution! I have marked this for inclusion in the
> next build.
> 
> Would you like any help finding something new to work on?

Thanks! I'm glad my patch is being included and I've made my first contribution. I would really like to do more!
Yes, please I'd like to work on something new. Any help is much appreciated!

Comment 17

[Chris H-C :chutten]

There are new bugs being filed as blocking bug 1423446 which would be good ones to look into if you're a fan of some more light cleanup in Telemetry.

If you're after something different, here are some rather more ancient bugs that were marked as good first bugs in Events (bug 282266) and Bookmarks (bug 302866). Let me know if you find something interesting!

Comment 18

[Swapnil P (:DarthSwap)]

I've looked into these and would like to discuss them further. Can I reach out to you on IRC?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3fbfc0454a
Removed probes GRADIENT_DURATION, GRADIENT_RETENTION_TIME. r=chutten

Comment 20

[Chris H-C :chutten]

Oh yes, please do feel free to reach out to me on IRC. You can find me on the #telemetry channel.

Comment 21

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ee3fbfc0454a