Closed
Bug 1391228
Opened 6 years ago
Closed 6 years ago
Check if devtools/shared/fronts/profiler.js can be removed
Categories
(DevTools :: Performance Tools (Profiler/Timeline), enhancement, P3)
DevTools
Performance Tools (Profiler/Timeline)
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•6 years ago
|
||
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.
Comment 5•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment 7•6 years ago
|
||
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.
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
(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
Assignee | ||
Comment 10•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
Updated patch removing actor, spec and front. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b0d0b6196efcff9996d299eb344dbc4d03a325c
Assignee | ||
Comment 13•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•6 years ago
|
||
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 16•6 years ago
|
||
mozreview-review |
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 17•6 years ago
|
||
mozreview-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+
Assignee | ||
Comment 18•6 years ago
|
||
Thanks for the reviews! Clearing the ni? for Victor as I believe that the answer from Markup clarifies the initial question.
Flags: needinfo?(vporof)
Comment 19•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•6 years ago
|
||
Rebased. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7618f4954b4183ee66d433dbfc3bdf4b3cf29096
Assignee | ||
Comment 22•6 years ago
|
||
Random errors on my try push. I don't believe they are related but started some retriggers.
Comment 23•6 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bf8dcf287da7 remove unused devtools profiler actor, specs and front;r=gregtatum,mstange
![]() |
||
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf8dcf287da7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•5 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•