Remove dead code from framerate actor now in GraphsWorker.js

RESOLVED FIXED in Firefox 40

Status

defect
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: jsantell, Assigned: wrahman0, Mentored)

Tracking

37 Branch
Firefox 40
x86
macOS

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

Comments in browser/devtools/shared/widgets/GraphsWorker.js indicate that code from framerate actor is now occuring in the worker, so we should remove the framerate actor code.
Blocks: perf-tool-v2
Mentor: jsantell
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 1

4 years ago
Could I be assigned this bug?
Sure thing! Let me know if you have any questions!
Assignee: nobody → wrahman0
(Assignee)

Comment 3

4 years ago
I have looked at the code and here is what I have done so far: 

1. I read the comments on GraphsWorker.js which redirected me to toolkit/devtools/server/actors/framerate.js. 
2. I skimmed through framerate.js and found the section that was copied. 
3. I have removed that section from framerate.js

Next Steps before Pull Request:
1. Run the tests to see if everything passes.
2. [Maybe] Look to see if unit tests exist for the function that I removed in framerate.js and transfer that over to the unit test section for GraphsWorker.js

I would love to hear what you think about my next steps.
Looking at it again, looks like the profiler (browser/devtools/profiler) is using the plotFPS function from the framerate actor. We've been working on a new version of the profiler (browser/devtools/performance) that's using this new GraphsWorker with the function. During the next uplift (a week or so), we'll be removing the profiler and timeline, for the performance tool to replace it. I don't think we'll be able to remove this code that the profiler is using until then, if you don't mind waiting until then.

In a week, the performance tool will be available to use without building with a build flag, as well.
Also to answer your question specifically -- sounds good! I don't think there are other uses of this, and running tests on the performance tool (once enabled) should be sufficient, and if there are any tests for the plotFPS function, lets migrate those over as well. Thanks!
(Assignee)

Comment 6

4 years ago
Yeah, it looks like the ui-profile.js is using plotFPS. I will wait until the next uplift before removing the redundant code from toolkit/devtools/server/actors/framerate.js. When its ready to be removed, just comment on this thread and let me know. There seems to be some testcases under toolkit/devtools/server/actors/tests/mochitest/test_framerate_01.html. Should this be migrated to browser/devtools/shared/test ?
Looks like only the first two tests (01, 02) test/use plotFPS -- these can probably stay there, just instantiate the GraphsWorker (look at browser/devtools/shared/widgets/Graphs.jsm to see how we use the ChromeWorker to instantiate it) to use the function there to plot FPS points.

Looks like it'll happen in a week or so, so I'll ping you once that happens! Thanks!
(Assignee)

Comment 8

4 years ago
Hey Jordan. I was just curious about the progress of the removal of the profiler and timeline? Would love to hear back.
(Assignee)

Comment 9

4 years ago
Hey Jordan. I was just curious about the progress of the removal of the profiler and timeline?
Flags: needinfo?(jsantell)
The new performance tool just landed in nightly, but there's a good chance of us holding it back for one more release (Fx40 nightly starts on March 30th). So I think it's good to start working on the patch and testing it (as the old profiler/timeline are disabled right now), but won't be able to land it for another 2 weeks -- how does that sound?
Flags: needinfo?(jsantell)
(Assignee)

Comment 11

4 years ago
That sounds good. I will send updates when I have them.
Not specifically blocking v2.
No longer blocks: perf-tool-v2
(Assignee)

Comment 13

4 years ago
I removed the code from framerate actor. I ran the tests after the removal and they all passed. However, test_framerate_01.html and test_framerate_02.html both refer to plotFPS. I dont understand why the testcases are still passing. I was hesitant on moving the tests. Please let me know what you think.
Flags: needinfo?(jsantell)
(Assignee)

Comment 14

4 years ago
This is just a patch in the case that the tests need not be transferred.
Attachment #8581375 - Flags: review?(jsantell)
Comment on attachment 8581375 [details] [diff] [review]
rev1 - Removed the dead code from framerate.js. Did not transfer the tests

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

Hey Wasiur -- we're finally ready to deprecate the old profiler which is being pushed up to Aurora today/tomorrow, so good to get going on this -- sorry for the wait!

Looking at the patch, looks like you got it -- I think the tests that are passing is because you need to rebuild the actors:

./mach build toolkit/devtools/server/

Then rerunning the tests should fail:

./mach mochitest toolkit/devtools/server/tests/mochitest/test_framerate_01.html
./mach mochitest toolkit/devtools/server/tests/mochitest/test_framerate_02.html

The tests are mostly testing that the framerate actor is working -- I think a fine solution to this would be to just put the `plotFPS` in both of these tests, as it's just a utility to more easily make sense of the FPS data. Once including the plotFPS in both of these tests (copied in both files, which isn't great, but they're tests and it's fine), rerunning the tests should pass!
Attachment #8581375 - Flags: review?(jsantell)
Flags: needinfo?(jsantell)
(Assignee)

Comment 16

4 years ago
Cool Jordan, thanks for the review. I like your solution and I dont think its much of an issue if we simply keep the tests where they are and just refer to the plotFPS function.
(Assignee)

Comment 17

4 years ago
I added the plotFPS function locally in each test case.
Attachment #8581375 - Attachment is obsolete: true
Attachment #8586542 - Flags: review?(jsantell)
Comment on attachment 8586542 [details] [diff] [review]
bug1132208_removeDeadCode-tests-passing.diff

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

::: toolkit/devtools/server/tests/mochitest/test_framerate_01.html
@@ +33,5 @@
>    SimpleTest.waitForExplicitFinish();
>  
>    var {FramerateFront} = devtools.require("devtools/server/actors/framerate");
>  
> +  FramerateFront.plotFPS = function(ticks, interval = 100, clamp = 60) {

Good! This might be better just being a function `plotFPS` rather than attaching it on the FramerateFront -- that could introduce some side effects. Once that's changed (in both tests), I think we'll be good to test on try.
Attachment #8586542 - Flags: review?(jsantell)
(Assignee)

Comment 19

4 years ago
I detached the plotFPS function from FramerateFront.
Attachment #8586542 - Attachment is obsolete: true
Attachment #8586565 - Flags: review?(jsantell)
Comment on attachment 8586565 [details] [diff] [review]
bug1132208_removeDeadCode-plotFPS-seperate-from-FramerateFront.diff

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

Looks great! pushing to our test servers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d704c47df308
Attachment #8586565 - Flags: review?(jsantell) → review+
Looks good, tests passing -- thanks wrahman0! Flagging this to be checked in for merge
Keywords: checkin-needed
(Assignee)

Comment 22

4 years ago
Thanks Jordan. Looking forward to fixing more bugs :D
https://hg.mozilla.org/integration/fx-team/rev/43e237f4a426
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/43e237f4a426
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 40

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.