Remove unused probes GRADIENT_DURATION and GRADIENT_RETENTION_TIME

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: padenot, Assigned: DarthSwap, Mentored)

Tracking

unspecified
mozilla59
Other
Other
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Taras, where could I find the relevant data to fix this?
Flags: needinfo?(taras.mozilla)
(Reporter)

Updated

6 years ago
Depends on: 811781

Updated

6 years ago
Flags: needinfo?(taras.mozilla) needinfo?(taras.mozilla) → needinfo+
(Reporter)

Comment 2

5 years ago
I don't plan to work on this in the near future.
Assignee: padenot → nobody
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.
Mentor: chutten
Priority: -- → P3
Summary: Use telemetry data for GRADIENT_DURATION and GRADIENT_RETENTION_TIME to choose proper value for the gradient cache. → Remove unused probes GRADIENT_DURATION and GRADIENT_RETENTION_TIME
Whiteboard: [good first bug][lang=c++]
(Assignee)

Comment 4

a year ago
Hi, I would like to work on this bug.
Excellent! I have assigned the bug to you. Please let me know if you have any questions.
Assignee: nobody → swapchamps
Status: NEW → ASSIGNED
(Assignee)

Comment 6

a year ago
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?
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.
(Assignee)

Comment 8

a year ago
Created attachment 8935824 [details] [diff] [review]
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.
Attachment #8935824 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8935824 - Flags: review+ → review?(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.
Attachment #8935824 - Flags: review?(chutten)
(Assignee)

Comment 10

a year ago
Created attachment 8936000 [details] [diff] [review]
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.
Attachment #8935824 - Attachment is obsolete: true
Attachment #8936000 - Flags: review?(chutten)
(Assignee)

Comment 11

a year ago
Hi! I am just following up on the patch review as I have not gotten any response. Thank You.
Flags: needinfo?(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.
Attachment #8936000 - Flags: review?(chutten)
(Assignee)

Comment 13

a year ago
Created attachment 8936772 [details] [diff] [review]
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.
Attachment #8936000 - Attachment is obsolete: true
Attachment #8936772 - Flags: review?(chutten)
Comment on attachment 8936772 [details] [diff] [review]
Bug785931.patch

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

Looks good, and builds locally without incident.
Attachment #8936772 - Flags: review?(chutten) → review+
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?
Flags: needinfo?(chutten)
Keywords: checkin-needed
(Assignee)

Comment 16

a year ago
(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!
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!
(Assignee)

Comment 18

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

Comment 19

a year ago
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3fbfc0454a
Removed probes GRADIENT_DURATION, GRADIENT_RETENTION_TIME. r=chutten
Keywords: checkin-needed
Oh yes, please do feel free to reach out to me on IRC. You can find me on the #telemetry channel.

Comment 21

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee3fbfc0454a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.