Closed Bug 1365400 Opened 7 years ago Closed 7 years ago

Support all profiler features in the geckoProfiler.start() WebExtension API

Categories

(WebExtensions :: Experiments, enhancement, P1)

Product:
WebExtensions
Component:
Experiments
Type:
enhancement

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Whiteboard: triaged)

Attachments

(2 files)

Bug 1365400 - Add all profiler features to the enum list, and a test.
59 bytes, text/x-review-board-request
kmag
: review+
n.nethercote
: review+
Details
Bug 1365400 - Add nsIProfiler::GetAllFeatures.
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details 
The geckoProfiler webextension API schema currently validates the "features" that get passed to geckoProfiler.start():

http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/browser/components/extensions/schemas/geckoProfiler.json#41-53

This list is missing a few items, most notably "mainthreadio". I don't think we get much value from having this check and I'd rather make this a free-form string, so that we don't have to remember to update this list when we add profiler features.
Blocks: 1326572
Part of the purpose the schema enum serves is documentation. Part is feature testing (schema values get added as properties to the type object in the geckoProfiler namespace).

If we're worried about compatibility, we can make unexpected entries a warning rather than an error by adding `"onError": "warn"` to the enum type.
Comment on attachment 8868307 [details]
Bug 1365400 - Add all profiler features to the enum list, and a test.

https://reviewboard.mozilla.org/r/139894/#review143276

::: tools/profiler/public/GeckoProfiler.h:156
(Diff revision 1)
>  
>  // Higher-order macro containing all the feature info in one place. Define
>  // |macro| appropriately to extract the relevant parts. Note that the number
>  // values are used internally only and so can be changed without consequence.
> +// Any changes to this list should also be applied to the feature list in
> +// browser/components/extensions/schemas/geckoProfiler.json.

Good comment. I wish JSON allowed comments.
Attachment #8868307 - Flags: review?(n.nethercote) → review+
Comment on attachment 8868307 [details]
Bug 1365400 - Add all profiler features to the enum list, and a test.

https://reviewboard.mozilla.org/r/139894/#review143280

::: browser/components/extensions/schemas/geckoProfiler.json:60
(Diff revision 1)
> +                    "restyle",
>                      "stackwalk",
>                      "tasktracer",
> -                    "leaf",
>                      "threads"
> -                  ]
> +                  ],

Can we export the list of features via nsIProfiler and add a test that this list matches its list, so we're sure they stay in sync?

We should also probably make this a type so extensions can check `if (browser.geckoProfiler.Features.GPU)` and so forth.

Other than that, looks good.
Attachment #8868307 - Flags: review?(kmaglione+bmo) → review-
nsIProfiler has a method GetFeatures which will, depending on the platform, return a subset of this list.
http://searchfox.org/mozilla-central/source/tools/profiler/gecko/nsIProfiler.idl#67

We can add a test that checks that GetFeatures returns a subset of the list we declare in the manifest.

Or would you prefer having another GetAllFeatures method so that we can check equality?
We should probably check equality against the full set, or we'll wind up eventually removing a feature and then forgetting to remove it from the schema.
Priority: -- → P2
Whiteboard: triaged
Issues that block the profiler add-on are P1. This is one of our most critically important tools, especially at the moment.
Priority: P2 → P1
(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #4)
> Comment on attachment 8868307 [details]
> Bug 1365400 - Add all profiler features to the enum list, and a test.
> 
> https://reviewboard.mozilla.org/r/139894/#review143280
> 
> ::: browser/components/extensions/schemas/geckoProfiler.json:60
> (Diff revision 1)
> > +                    "restyle",
> >                      "stackwalk",
> >                      "tasktracer",
> > -                    "leaf",
> >                      "threads"
> > -                  ]
> > +                  ],
> 
> Can we export the list of features via nsIProfiler and add a test that this
> list matches its list, so we're sure they stay in sync?

Done. Is this what you had in mind? The test has the path "chrome://browser/content/schemas/geckoProfiler.json" hardcoded in it. It also uses Schemas.parseSchemas() and then grovels through the returned structure to find what it needs. Should I be using Schemas.inject() instead?

> We should also probably make this a type so extensions can check `if
> (browser.geckoProfiler.Features.GPU)` and so forth.

Would this make the existing add-on incompatible with the updated API? I'd like to avoid that.
Summary: Don't validate profiler features at the WebExtension API level → Support all profiler features in the geckoProfiler.start() WebExtension API
Comment on attachment 8877711 [details]
Bug 1365400 - Add nsIProfiler::GetAllFeatures.

https://reviewboard.mozilla.org/r/149148/#review153730

::: tools/profiler/gecko/nsProfiler.cpp:461
(Diff revision 1)
> +}
> +
> +NS_IMETHODIMP
> +nsProfiler::GetAllFeatures(uint32_t* aCount, char*** aFeatureList)
> +{
> +  GetArrayOfStringsForFeatures((uint32_t)-1, aCount, aFeatureList);

Is it worth adding a profiler_get_all_features() function? Not sure.
Attachment #8877711 - Flags: review?(n.nethercote) → review+
I would dearly love to get rid of these arrays of strings.
(In reply to Nicholas Nethercote [:njn] from comment #12)
> Comment on attachment 8877711 [details]
> Bug 1365400 - Add nsIProfiler::GetAllFeatures.
> 
> https://reviewboard.mozilla.org/r/149148/#review153730
> 
> ::: tools/profiler/gecko/nsProfiler.cpp:461
> (Diff revision 1)
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsProfiler::GetAllFeatures(uint32_t* aCount, char*** aFeatureList)
> > +{
> > +  GetArrayOfStringsForFeatures((uint32_t)-1, aCount, aFeatureList);
> 
> Is it worth adding a profiler_get_all_features() function? Not sure.

I considered it, but since the caller needs to use the feature iteration macro anyway, I didn't think it would be much of an improvement.

(In reply to Nicholas Nethercote [:njn] from comment #13)
> I would dearly love to get rid of these arrays of strings.

And have some constants at the IDL level? I suppose that would be preferable.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
This bug is currently blocking some main thread IO profiling work that the Photon flow people want to do.
(In reply to Markus Stange [:mstange] from comment #11)
> > We should also probably make this a type so extensions can check `if
> > (browser.geckoProfiler.Features.GPU)` and so forth.
>
> Would this make the existing add-on incompatible with the updated API? I'd
> like to avoid that.

No. The API would look the same, we'd just get an additional type object in
the namespace that extension code can reference. It would look something like
this:

    "types": [
      {
        "name": "ProfilerFeature",
        "type": "string",
        "enum": [
          "js",
          "stackwalk",
          "tasktracer",
          "leaf",
          "threads",
          ...
        ]
      }
    ],

    ...
              "features": {
                "type": "array",
                "description": "A list of active features for the profiler.",
                "items": {
                  "$ref": "ProfilerFeature"
                }
              },
Comment on attachment 8868307 [details]
Bug 1365400 - Add all profiler features to the enum list, and a test.

https://reviewboard.mozilla.org/r/139894/#review154166

r=me, but I'd still rather we add a ProfilerFeature type, which would make the test much easier, since we'd be able to just check `Object.values(browser.geckoProfiler.ProfilerFeature)`
Attachment #8868307 - Flags: review?(kmaglione+bmo) → review+
Ooh, I like that.
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/2573b20f6f90
Add nsIProfiler::GetAllFeatures. r=njn
https://hg.mozilla.org/integration/autoland/rev/6ac2d61a3904
Add all profiler features to the enum list, and a test. r=kmag,njn
https://hg.mozilla.org/mozilla-central/rev/2573b20f6f90
https://hg.mozilla.org/mozilla-central/rev/6ac2d61a3904
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: