Closed Bug 1132208 Opened 6 years ago Closed 6 years ago

Remove dead code from framerate actor now in GraphsWorker.js


(DevTools :: Performance Tools (Profiler/Timeline), defect)

37 Branch
Not set


(firefox40 fixed)

Firefox 40
Tracking Status
firefox40 --- fixed


(Reporter: jsantell, Assigned: wrahman0, Mentored)


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


(1 file, 2 obsolete files)

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]
Could I be assigned this bug?
Sure thing! Let me know if you have any questions!
Assignee: nobody → wrahman0
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!
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!
Hey Jordan. I was just curious about the progress of the removal of the profiler and timeline? Would love to hear back.
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)
That sounds good. I will send updates when I have them.
Not specifically blocking v2.
No longer blocks: perf-tool-v2
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)
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)
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.
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]

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)
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]

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

Looks great! pushing to our test servers:
Attachment #8586565 - Flags: review?(jsantell) → review+
Looks good, tests passing -- thanks wrahman0! Flagging this to be checked in for merge
Keywords: checkin-needed
Thanks Jordan. Looking forward to fixing more bugs :D
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.