1444796 - perfActor.startProfiler should have a option for entries, internal and etc

Status: RESOLVED FIXED

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

Description

[Makoto Kato [:m_kato]]

Actually, startProfiler doesn't have the option.  When calling Services.profiler.StartProfiler, it uses default option.  But if we support remote Gecko profiler, we should be able to control it.

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1444796 - Part 1. Add options to startProfiler to customize profiler parameter from remote.

New remote profiler protocol uses perfActor, but startProfiler doesn't support
parameters such as interval, So perfActor should support parameters for
startProfiler.

Review commit: https://reviewboard.mozilla.org/r/227642/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227642/

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1444796 - Part 2. Make nsIProfilerStartParams scriptable.

To add test for startProfiler's parameters on devtools, nsIProfilerStartParams
changes to scriptable.

Review commit: https://reviewboard.mozilla.org/r/227644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227644/

Comment 3

[Makoto Kato [:m_kato]]

Attachment: Bug 1444796 - Part 3. Add test for parameters of startProfiler.

nsIProfiler emit profiler-started event with parameters, but perfActor doesn't
set the parameter yet.  For unit test, we should set parameter on
profiler-started event.

Then, unit test for startProfiler's parameters should use it.

Review commit: https://reviewboard.mozilla.org/r/227646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227646/

Comment 4

[Greg Tatum [:gregtatum]]

Comment on attachment 8958731 [details]
Bug 1444796 - Part 3. Add test for parameters of startProfiler.

https://reviewboard.mozilla.org/r/227646/#review233526

::: devtools/server/tests/browser/browser_perf-04.js:27
(Diff revision 1)
> +    is(interval, 0.1, "Should apply interval by startProfiler");
> +    is(features, 0x82, "Should apply features by startProfiler");
> +  });
> +
> +  // Start the profiler.
> +  await front.startProfiler({ entries: 1000, interval: 0.1,

Thanks for adding a test!

::: devtools/shared/specs/perf.js:14
(Diff revision 1)
>    typeName: "perf",
>  
>    events: {
>      "profiler-started": {
> -      type: "profiler-started"
> +      type: "profiler-started",
> +      entries: Arg(0, "number"),

Should this change be on the first commit, since it's affecting the actor interface?

Comment 5

[Greg Tatum [:gregtatum]]

Comment on attachment 8958731 [details]
Bug 1444796 - Part 3. Add test for parameters of startProfiler.

https://reviewboard.mozilla.org/r/227646/#review233526

Thanks for adding this feature. We should probably follow-up with some UI changes as well. Should I do some UI mockups?

Comment 6

[Greg Tatum [:gregtatum]]

Comment on attachment 8958729 [details]
Bug 1444796 - Part 1. Add options to startProfiler to customize profiler parameter from remote.

https://reviewboard.mozilla.org/r/227642/#review233546

My only real comment is on commit ordering, see my comment on the other commit.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8958730 [details]
Bug 1444796 - Part 2. Make nsIProfilerStartParams scriptable.

https://reviewboard.mozilla.org/r/227644/#review233594

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

Is this work blocked on anything or can it land?

Comment 9

[Makoto Kato [:m_kato]]

(In reply to Panos Astithas [:past] (please ni?) from comment #8)
> Is this work blocked on anything or can it land?

Oh, I forget this.  I will land this tomorrow.

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8958731 [details]
Bug 1444796 - Part 3. Add test for parameters of startProfiler.

https://reviewboard.mozilla.org/r/227646/#review233526

If it is available, it is better.  It is unncessary at this time.

> Should this change be on the first commit, since it's affecting the actor interface?

Before part 2, we cannot get any parameters from profiler-started event.  For unit test, I change nsIProfilerStartParams interface as scriptable (this is part 2).  After part 2, we can add parameter information such as entries to thie event.

Comment 11

[Makoto Kato [:m_kato]]

If I should merge actor change to 1st patch, it swap part 1 (actor change) with part 2 (XPCOM change), then, merge it with new part 2 (actor change).

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c1967a96814
Part 1. Add options to startProfiler to customize profiler parameter from remote. r=gregtatum
https://hg.mozilla.org/integration/mozilla-inbound/rev/28132fc715f7
Part 2. Make nsIProfilerStartParams scriptable. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd4ba836876
Part 3. Add test for parameters of startProfiler. r=gregtatum

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8c1967a96814
https://hg.mozilla.org/mozilla-central/rev/28132fc715f7
https://hg.mozilla.org/mozilla-central/rev/5cd4ba836876