1132208 - Remove dead code from framerate actor now in GraphsWorker.js

Status: RESOLVED FIXED

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

Description

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 1

[wrahman0]

Could I be assigned this bug?

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Sure thing! Let me know if you have any questions!

Comment 3

[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.

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

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!

Comment 6

[wrahman0]

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 ?

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

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!

Comment 8

[wrahman0]

Hey Jordan. I was just curious about the progress of the removal of the profiler and timeline? Would love to hear back.

Comment 9

[wrahman0]

Hey Jordan. I was just curious about the progress of the removal of the profiler and timeline?

Comment 10

[Jordan Santell [:jsantell] [@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?

Comment 11

[wrahman0]

That sounds good. I will send updates when I have them.

Comment 12

[Victor Porof [:vporof][:vp]]

Not specifically blocking v2.

Comment 13

[wrahman0]

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.

Comment 14

[wrahman0]

Attachment: rev1 - Removed the dead code from framerate.js. Did not transfer the tests

This is just a patch in the case that the tests need not be transferred.

Comment 15

[Jordan Santell [:jsantell] [@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!

Comment 16

[wrahman0]

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.

Comment 17

[wrahman0]

Attachment: bug1132208_removeDeadCode-tests-passing.diff

I added the plotFPS function locally in each test case.

Comment 18

[Jordan Santell [:jsantell] [@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.

Comment 19

[wrahman0]

Attachment: bug1132208_removeDeadCode-plotFPS-seperate-from-FramerateFront.diff

I detached the plotFPS function from FramerateFront.

Comment 20

[Jordan Santell [:jsantell] [@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

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

Looks good, tests passing -- thanks wrahman0! Flagging this to be checked in for merge

Comment 22

[wrahman0]

Thanks Jordan. Looking forward to fixing more bugs :D

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/43e237f4a426

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/43e237f4a426