Remove unused probes GRADIENT_DURATION and GRADIENT_RETENTION_TIME

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
6 years ago
10 months 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

Comment 3

11 months ago
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

11 months ago
Hi, I would like to work on this bug.

Comment 5

11 months ago
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

11 months 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?

Comment 7

11 months ago
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

11 months 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

11 months ago
Attachment #8935824 - Flags: review+ → review?(chutten)

Comment 9

11 months ago
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

11 months 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

10 months ago
Hi! I am just following up on the patch review as I have not gotten any response. Thank You.
Flags: needinfo?(chutten)

Comment 12

10 months ago
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

10 months 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 14

10 months ago
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+

Comment 15

10 months ago
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

10 months 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!

Comment 17

10 months ago
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

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

Comment 19

10 months 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

Comment 20

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

Comment 21

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee3fbfc0454a
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months 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.