Closed Bug 1391228 Opened 7 years ago Closed 7 years ago

Check if devtools/shared/fronts/profiler.js can be removed

Categories

(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)

enhancement

Tracking

(firefox57 fixed)

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

Details

Attachments

(1 file)

I think that devtools/shared/fronts/profiler.js is no longer used in devtools.

It is only imported in two tests: 
- devtools/server/tests/unit/test_profiler_events-01.js
- devtools/server/tests/unit/test_profiler_events-02.js

When looking at code coverage for this file:
https://codecov.io/gh/marco-c/gecko-dev/src/master/devtools/shared/fronts/profiler.js

we can see it is only imported twice, which most likely corresponds to the 2 tests mentioned above.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
While discussing this with Sole, she found https://bugzilla.mozilla.org/show_bug.cgi?id=1265727#c13 which seems to indicate that the file was already considered for removal when we moved fronts to shared. It was kept for potential b2g compatibility, but I think we can safely remove it now.
I have not looked through the code to verify the assumptions listed here, but I would say that anything kept around for legacy reasons like b2g would be a good candidate for removal, especially with our future plans to integrate perf.html with devtools.
Shouldn't we also be able to remove the spec and actor corresponding to this front? It appears we may not be able to from https://dxr.mozilla.org/mozilla-central/source/devtools/server/main.js#551-562, but I don't know why we are using the actor but not the front.
Maybe it can be removed from devtools/server/main after all, there aren't other references that I can see (except for some Servo actors - and I'm not sure if those are currently working and/or would break from the front removal): https://dxr.mozilla.org/mozilla-central/search?q=PerformanceActor&redirect=true.

Victor, do you know if the PerformanceActor is only around for backwards compatibility or for some other reason?
Flags: needinfo?(vporof)
(In reply to Brian Grinstead [:bgrins] from comment #7)
> Shouldn't we also be able to remove the spec and actor corresponding to this
> front? It appears we may not be able to from
> https://dxr.mozilla.org/mozilla-central/source/devtools/server/main.js#551-
> 562, but I don't know why we are using the actor but not the front.

Thanks! Looks like you are right, we can probably get rid of the actor, specs and related tests. We have a bunch of tests in devtools based on `target.hasActor("profiler")` [1] but we can probably replace that with target.hasActor("performance")

[1] https://dxr.mozilla.org/mozilla-central/search?q=hasActor(%22profiler%22)&redirect=false
Comment on attachment 8898832 [details]
Bug 1391228 - remove unused devtools profiler actor, specs and front;

Clearing the review request for now, waiting for Victor's feedback. Will attach a new patch after that.
Attachment #8898832 - Flags: review?(gtatum)
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Maybe it can be removed from devtools/server/main after all, there aren't
> other references that I can see (except for some Servo actors - and I'm not
> sure if those are currently working and/or would break from the front
> removal):
> https://dxr.mozilla.org/mozilla-central/
> search?q=PerformanceActor&redirect=true.
> 

Asked in #servo about their Profiler actors. 

Found a few dependencies additional to the Profiler actor in xpcshell tests:
- devtools/server/tests/unit/test_profiler_activation-01.js
- devtools/server/tests/unit/test_profiler_activation-02.js
- devtools/server/tests/unit/test_profiler_bufferstatus.js
- devtools/server/tests/unit/test_profiler_close.js
- devtools/server/tests/unit/test_profiler_data.js
- devtools/server/tests/unit/test_profiler_getbufferinfo.js
- devtools/server/tests/unit/test_profiler_getfeatures.js
- devtools/server/tests/unit/test_profiler_sharedlibraries.js

Markus: looking at the tests, they seem to only be asserting the profiler actor itself (response type is correct etc...), so I don't think they provide valuable coverage to the underlying profiler. Can you have a look and let me know if you're fine with us removing them?
Flags: needinfo?(mstange)
Try is green at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a725096c0eb37a3c4448e4649f0adb4a192fb497 

Markus: clearing ni? as I flagged you for review (mostly to make sure deleting our tests is ok)
Flags: needinfo?(mstange)
Comment on attachment 8898832 [details]
Bug 1391228 - remove unused devtools profiler actor, specs and front;

https://reviewboard.mozilla.org/r/170214/#review179652

Looks good to me. It's a little concerning that we don't have unit tests for the performance actor, but this patch doesn't really affect that one way or the other. The performance panel has full integration tests that actually run the profiler. The Gecko Profiler mainly uses these tests as sanity checks, so I don't think removing these will be an issue on that front.
Attachment #8898832 - Flags: review?(gtatum) → review+
Comment on attachment 8898832 [details]
Bug 1391228 - remove unused devtools profiler actor, specs and front;

https://reviewboard.mozilla.org/r/170214/#review179940

The last use of the profiler actor was here: https://github.com/devtools-html/Gecko-Profiler-Addon/blob/4e14f1447f58197c127aff476d82d95da4ca8500/lib/remote-profiler.js#L14

It was used in the pre-webextension Gecko profiler add-on in order to control the profiler in Firefox for Android.

But this code is now gone, because there is no webextension API to connect to a remote debugging target, and as a result, the Gecko profiler add-on can't be used to profile Firefox for Android.
Once the relevant webextensions APIs are added, we will probably need to add some new methods to the performance actor, in order to get the information that the Gecko Profiler needs.
Attachment #8898832 - Flags: review?(mstange) → review+
Thanks for the reviews! 
Clearing the ni? for Victor as I believe that the answer from Markup clarifies the initial question.
Flags: needinfo?(vporof)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 5e0fffb16aff -d e066d2f8f62d: rebasing 417322:5e0fffb16aff "Bug 1391228 - remove unused devtools profiler actor, specs and front;r=gregtatum,mstange" (tip)
local [dest] changed devtools/server/actors/profiler.js which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
local [dest] changed devtools/shared/fronts/profiler.js which other [source] deleted
use (c)hanged version, (d)elete, or leave (u)nresolved? u
merging devtools/client/framework/toolbox.js
merging devtools/docs/backend/backward-compatibility.md
merging devtools/server/actors/moz.build
merging devtools/server/main.js
merging devtools/shared/fronts/moz.build
merging devtools/shared/specs/moz.build
unresolved conflicts (see hg resolve, then hg rebase --continue)
Random errors on my try push. I don't believe they are related but started some retriggers.
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf8dcf287da7
remove unused devtools profiler actor, specs and front;r=gregtatum,mstange
https://hg.mozilla.org/mozilla-central/rev/bf8dcf287da7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.