Closed Bug 1326572 Opened 7 years ago Closed 7 years ago

Provide an API for nsIProfiler

Categories

(WebExtensions :: Experiments, defect)

49 Branch
defect
Not set
normal

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mconley, Assigned: alexical)

References

(Depends on 1 open bug)

Details

(Whiteboard: triaged)

Attachments

(1 file)

The Gecko Profiler Add-on[1] is used to retrieve performance profile data from a running Firefox instance. It is extremely useful for identifying bottlenecks in our code, and for reasoning about why things might be happening slower than you'd like.

The Gecko Profiler Add-on is really just a front-end for the nsIProfiler service[2]. The service is what the add-on uses to turn on profiling, configure profiling, capture profiles, and stop profiling.

If we want the Gecko Profiler Add-on to work post 2017, we'll need to make it into a WebExtension. This will mean designing and writing a new API that exposes what we need from nsIProfiler.

[1]: https://github.com/mstange/gecko-profiler-addon
[2]: http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/tools/profiler/gecko/nsIProfiler.idl
looking for input from performance team on this change
Flags: needinfo?(hkirschner)
:shell, we have some projects that depend on the profiler addon, not just developers but also making it easier to report performance issues for users.

+Bryan, we talked about providing a debugger web extensions API as client for the chrome debugging protocol [1].

+Jim, who is working on the new chrome debugger protocol server.

[1]: https://developer.chrome.com/extensions/debugger
Flags: needinfo?(jimb)
Flags: needinfo?(hkirschner)
Flags: needinfo?(clarkbw)
Yes, we need this.  BTW, the profiler add-on has moved here: https://github.com/devtools-html/Gecko-Profiler-Addon old links should redirect.

bug 1211859 is tracking the work for devtools / webextensions in general.
Flags: needinfo?(clarkbw)
Anyone willing to work on this? We can provide WebExtensions help and support, but not high on our priority list compared to other things.

Also, should access to this API be restricted in any way? For example, is it expensive to query or contain sensitive information?
Whiteboard: triaged
(In reply to Andy McKay [:andym] from comment #4)
> Also, should access to this API be restricted in any way? For example, is it
> expensive to query or contain sensitive information?

Expensive to query: Kinda. Each profiled thread has to be temporarily paused to dump its profile samples. This is usually pretty quick, but it definitely shouldn't be done in a busy loop or anything.

There's also sensitive information in there. We get pseudostacks here, meaning JS execution stacks, which might point at some URLs that the user is browsed to, as well as internal script URLs for add-ons that the user is running. We also have things like library versions and hardware stats in a profile.

A profile could be used, in theory, to identify the user who sent it. We don't do much to sanitize it at all.
Thanks. At the very least then this should be behind a permission, so the user installing will be prompted with a warning telling them what this extension can do.
> should be behind a permission

Definitely, outside of performance profiling work no one should really be accessing this API.
Is this on the roadmap for before 57?
Flags: needinfo?(clarkbw)
Not sure, punting this to Q2.  If we get started early we might be able to squeeze it into 56 or 57.  

Harald: Is there a need for 57?

Perhaps the next get together should include Andy or someone from his team to help us sprint on getting this done.
Flags: needinfo?(clarkbw) → needinfo?(hkirschner)
> Is there a need for 57?

As far as I understand, the Profiler addon will not work past Firefox 57 and needs to be converted to a web extension. To make that work, we probably need the necessary APIs.
Flags: needinfo?(hkirschner)
This seems like another add-on that could take the privileged signing approach (still under discussion) if it's not possible to complete the WebExtension API by 57.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> This seems like another add-on that could take the privileged signing
> approach (still under discussion) if it's not possible to complete the
> WebExtension API by 57.

I'm not sure it could. We were planning to remove most of the add-on SDK code from the tree in 57. I'd rather not change those plans for the sake of the profiler add-on, especially since the SDK version of that add-on has particularly bad performance issues.

It should be simple enough to implement a WebExtension API for the profiler. It might be worth starting it off as an experiment[1].

[1]: http://webextensions-experiments.readthedocs.io/en/latest/experiments/index.html
> It should be simple enough to implement a WebExtension API for the profiler. It might be worth starting it off as an experiment[1].

Given that one of our key performance tools should not just stop working with 57, who would own adding that experimental profiler API?
Flags: needinfo?(jimb) → needinfo?(amckay)
I believe mstange is already working on this.
Flags: needinfo?(amckay) → needinfo?(jimb)
Not really... you could even say I'm working away from it, because I'm adding more features to the SDK-based add-on. But the main goal of this is to gather information about what functionality we'll need from the WebExtension API.

Here's a breakdown of what I think we'll need:

 - General profiler control: start, stop, pause, resume, collectProfile, addMarker
 - Symbolication: getSymbolTableForLibrary(debugName, breakpadId)
 - Shutdown and startup profiling: triggerRestartAndGatherProfiles({ shutdown: { /* shutdown profiler settigs */ }, startup: { /* startup profiler settings */ } }), getShutdownProfileIfAvailable(), getStartupProfileIfAvailable()
 - And all of the above but for a remote Firefox for Android debugger target

I'm currently experimenting with getting profiles from Firefox for Android. My current approach is to add a devtools panel to WebIDE that uses the ProfilerActor over the debugger protocol.

getSymbolTableForLibrary will be rather interesting to implement. Currently, it does the following:
 - If this Firefox process is running from an objdir, and there's a symbol dump in the objdir for this library, use it.
 - Else, if the symbol dump is available on symbols.mozilla.org, download and parse it.
 - Else, if on Linux, and the command line tool "nm" is available, get a symbol dump from nm and parse it.
 - Else, if on Linux, read the binary into memory and run the Linux version of dump_syms (compiled to asm.js) on it.
 - Else, if on Mac, read the binary into memory and run the Mac version of dump_syms (compiled to asm.js) on it.

Additionally, for local builds running on Android it will need to do:
 - If the objdir on the host machine for this build is known, find the symbol dump in there or run dump_syms on the binary in the objdir.

And for system libraries on Android:
 - Pull the library from the device using adb, and run dump_syms on it.
Flags: needinfo?(jimb)
(In reply to Markus Stange [:mstange] from comment #15)
> Not really... you could even say I'm working away from it, because I'm
> adding more features to the SDK-based add-on. But the main goal of this is
> to gather information about what functionality we'll need from the
> WebExtension API.

If that's your goal, you should probably start with a WebExtensions experiment rather than adding more features to the SDK-based extension. The sooner we get away from overhead from SDK gunk intruding on profiles, the better.
Hey kmag,

Are there known SDK modules that we should definitely avoid[1] because they add overhead? Like sdk/tabs or sdk/simple-prefs?

Here's what the add-ons index.js is using: https://github.com/mstange/Gecko-Profiler-Addon/blob/master/index.js#L1-L12

Because perhaps we can swap some of these out for WebExtension APIs using the embedded WE stuff.

[1]: "All of them" is acceptable in terms of commiseration, though I guess it doesn't really helps us remove noise from the profiles that will become more and more important over the next few weeks.
Flags: needinfo?(kmaglione+bmo)
It would be easier to come up with a list of APIs that don't have performance concerns than those that do.

Pretty much all of the UI modules have major performance issues. Anything that cause content modules to be loaded is a problem. Until bug 1314861 is fixed, basically even just loading an SDK extension is a problem, because the extension loader modules load a huge dependency tree immediately, and trigger a bunch of unnecessary overhead.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #18)
> It would be easier to come up with a list of APIs that don't have
> performance concerns than those that do.

I'll take whatever I can get. :)

Are there any of those safe-to-use APIs in https://github.com/mstange/Gecko-Profiler-Addon/blob/master/index.js#L1-L12 ?
Flags: needinfo?(kmaglione+bmo)
Just to check that the expected lifespan of the SDK is not very long and in 57 they won't be allowed on release. Didn't you to spend too much time worrying about something that we don't need to care about for very much longer.
All of these are definitely problems:

const { Hotkey } = require("sdk/hotkeys");
const tabs = require("sdk/tabs");
const { viewFor } = require("sdk/view/core");
const { getBrowserForTab } = require("sdk/tabs/utils");
const { ToggleButton } = require('sdk/ui/button/toggle');
const { Panel } = require("sdk/panel");

This one *might* be OK, if not for the fact that just loading an SDK extension is a problem:

const { prefs } = require("sdk/simple-prefs");
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → dothayer
Markus, is there anything I can do to help get this converted to a WebExtension? Creating an experiments API extension for the profiler shouldn't be too much work, but looking at the SDK version, it's hard to tell exactly what it needs or what all of the parts are doing. I can't even figure out where the emscripten worker code comes from, so far...
Flags: needinfo?(mstange)
Ah, I missed the reassignment. So feel free to redirect the needinfo to dthayer, I guess.
Doug's wip is at https://github.com/squarewave/gecko-profiler-webextension and it's fairly feature-complete, as far as I can tell.

The instructions for generating the emscripten worker code are still only in a text file on my machine, unfortunately. :(
I need to polish up the patch and write proper instructions.
Flags: needinfo?(mstange)
Yeah, this should be at feature parity with the existing extension now. @mstange do you have any suggestions on who should be reviewing what for this? Also, do you want me to go ahead and tackle shutdown/restart profiling or should I wait on that?
Flags: needinfo?(mstange)
I can review the extension API portions.
Kris, how would you like to conduct the review? Should I make a patch for this to land in the tree, or do we normally keep experiments separate? Currently all the code for the profiler API is under here: https://github.com/squarewave/gecko-profiler-webextension/tree/master/api/profiler
Flags: needinfo?(kmaglione+bmo)
I can do a pre-review on Github if you want, but we should really probably just land it in-tree.
Flags: needinfo?(kmaglione+bmo)
I'd also like to review.

I'd like to understand what compatibility promises a WebExtension API entails specifically. Will any API that we add here need to exist forever? Can we change the properties of the result values? E.g. we'll want to make changes to the profile format fairly regularly.

The WebIDE panel part should probably be reviewed by somebody who has been involved in the other DevTools WebExtension API work.

We have a few features that we'd like to implement at some point, and we'd need to take those into account if these APIs will be set in stone. Those are, of the top of my head:
 - Restart profiling (startup + shutdown)
 - Exposing more information in the symbol tables, like filename + line number info, and information about inlined functions
 - The ability for the user to specify the objdir directory that they used for an Android build, so that we can get symbols from it
 - The ability for the user to specify the path to their Android NDK tools, so that we can find the appropriate "nm" binary
 - Some way of streaming the data to perf.html that doesn't involve so much work on the main threads.
Flags: needinfo?(mstange)
I think we'll want to limit this API to a whitelisted set of extensions, at least at the start, which means that we can be fairly liberal about making changes to it.

WebIDE is going away, so I'd rather not build support for it into any API. If we do, though, it should be a separate devtools API, not part of the profiler API.
(In reply to Kris Maglione [:kmag] from comment #30)
> WebIDE is going away, so I'd rather not build support for it into any API.
> If we do, though, it should be a separate devtools API, not part of the
> profiler API.

(For those following along, WebIDE is being replaced by about:debugging, see bug 1314811.)

I agree, it shouldn't be part of the profiler API.
The current add-on doesn't really know anything about WebIDE; it just registers a new devtools panel and declares itself to be only available for remote targets. So any API we create for that should work regardless of whether that panel is shown in WebIDE or in about:debugging.

I think the devtools API that we need can be limited to just "give me a profiler object for the target that I'm attached to", and that object would have the same API as the profiler API that we're designing here.
(I'm continually too slow to respond to these.) Do we really want a separate devtools API rather than fixing bug 1304378, or is that essentially what you're referring to?

On a separate note, if we're landing this in tree, it would be helpful to see the documentation for generating the asm.js code, since I'm not sure it makes sense to include asm.js in tree instead of just calling native code.
(In reply to Doug Thayer [:dthayer] from comment #32)
> (I'm continually too slow to respond to these.) Do we really want a separate
> devtools API rather than fixing bug 1304378

No, I think fixing bug 1304378, and having a devtools API that gives you access to a profiler API, should be all that's needed.

> On a separate note, if we're landing this in tree, it would be helpful to
> see the documentation for generating the asm.js code, since I'm not sure it
> makes sense to include asm.js in tree instead of just calling native code.

Good point, I think we should just call native code at that point. We already have the code in-tree at http://searchfox.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/tools/mac/dump_syms/dump_syms_tool.cc#189 and http://searchfox.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/common/linux/dump_symbols.cc#1094 . The add-on has the advantage that it can dump both Linux and Mac binaries on any platform; a straightforward use of the breakpad code from C++ code in Firefox would only be able to dump binaries from the platform that Firefox is being built for. But that's usually what you want anyway. Except maybe if you want to use the Linux dump_syms code to dump symbols from Android binaries on Mac. But I think with a bit of build system wrangling that should be possible, too.
(In reply to Markus Stange [:mstange] from comment #33)
> No, I think fixing bug 1304378, and having a devtools API that gives you
> access to a profiler API, should be all that's needed.

Alright. Given that, should I just implement what we need of the devtools API for remote tabs, and create a separate linked bug to track it? It's definitely going to be more work with more back and forth to support any more than just providing a panel and a profiler object. I'm willing to do it but I know we wanted to get this out quickly.
Flags: needinfo?(mstange)
Yes, let's have a separate bug for the remote profiler API, and focus on the local profiler API first.
Flags: needinfo?(mstange)
Comment on attachment 8858343 [details]
Bug 1326572 - Provide an API for nsIProfiler

https://reviewboard.mozilla.org/r/130296/#review133022

::: browser/components/extensions/ExtensionPopups.jsm:1
(Diff revision 1)
> -/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +./* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

Nit: Remove `.`

::: browser/components/extensions/ParseSymbols.jsm:7
(Diff revision 1)
> +  if (text instanceof Uint8Array) {
> +    let decoder = new TextDecoder("utf-8");
> +    return decoder.decode(text);
> +  }

Please make this an ArrayBuffer.

::: browser/components/extensions/ParseSymbols.jsm:11
(Diff revision 1)
> +  if (text instanceof Blob) {
> +    let fileReader = new FileReaderSync();
> +    return fileReader.readAsText(text, "utf-8");
> +  }

Is this used?

::: browser/components/extensions/ParseSymbols.jsm:34
(Diff revision 1)
> +      const addrStart = nextPublic + '\nPUBLIC '.length;
> +      const addrEnd = text.indexOf(' ', addrStart);
> +      const address = parseInt(text.substring(addrStart, addrEnd), 16);
> +      const symStart = text.indexOf(' ', addrEnd + 1) + 1;
> +      const symEnd = text.indexOf('\n', symStart);

Can we use a regexp for this? The JIT might be able to handle this pretty well, but the regexp JIT will almost certainly handle it better.

::: browser/components/extensions/ext-browser.js:183
(Diff revision 1)
>      manifest: ["page_action"],
>      paths: [
>        ["pageAction"],
>      ],
>    },
> +  profiler: {

Let's call this "geckoProfiler" for now. We may want to add a more stable/less privileged web content profiler API in the future, so we should try to avoid that conflict.

Also, we'll need to restrict this to a whitelisted set of add-ons. Ideally, we should enforce that via AMO signing, but for now, let's just restrict it to a specific set of add-on IDs.

::: browser/components/extensions/ext-profiler.js:1
(Diff revision 1)
> +Cu.import("resource://gre/modules/Services.jsm");

Please add "use strict", and the same license and modeline comments we have in other APIs.

::: browser/components/extensions/ext-profiler.js:2
(Diff revision 1)
> +Cu.import("resource://gre/modules/Subprocess.jsm");
> +Cu.import("resource:///modules/ParseSymbols.jsm");
> +const { OS } = Cu.import("resource://gre/modules/osfile.jsm", {});
> +const { gDevTools } = Cu.import("resource://devtools/client/framework/gDevTools.jsm", {});

Lazy import these, please.

::: browser/components/extensions/ext-profiler.js:8
(Diff revision 1)
> +XPCOMUtils.defineLazyGetter(this, "gProfiler", function() {
> +  return Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);
> +});

`XPCOMUtils.defineLazyServiceGetter`

Except this already exists as Services.profiler, so just remove it.

::: browser/components/extensions/ext-profiler.js:16
(Diff revision 1)
> +
> +const kAsyncStackPrefName = "javascript.options.asyncstack";
> +const gAsyncStacksWereEnabled = Services.prefs.getBoolPref(kAsyncStackPrefName, false);
> +
> +function parseSym(data) {
> +  const worker =  new Worker('resource://app/modules/ParseSymbols-worker.js');

Double-quoted strings, please. Same elsewhere.

::: browser/components/extensions/ext-profiler.js:17
(Diff revision 1)
> +const kAsyncStackPrefName = "javascript.options.asyncstack";
> +const gAsyncStacksWereEnabled = Services.prefs.getBoolPref(kAsyncStackPrefName, false);
> +
> +function parseSym(data) {
> +  const worker =  new Worker('resource://app/modules/ParseSymbols-worker.js');
> +  worker.postMessage(data);

Presumably we want to transfer this data rather than clone it.

::: browser/components/extensions/ext-profiler.js:18
(Diff revision 1)
> +  return new Promise((resolve, reject) => {
> +    worker.onmessage = (e) => {
> +      if (e.data.error) {
> +        reject(e.data.error);
> +      } else {
> +        resolve(e.data.result);
> +      }

Do we really want to spawn a new worker each time we call this? Either way, please explicitly close the worker when you're done with it.

::: browser/components/extensions/ext-profiler.js:29
(Diff revision 1)
> +function getPlatform() {
> +  return Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime).OS;
> +}

`Services.appinfo.OS`

::: browser/components/extensions/ext-profiler.js:44
(Diff revision 1)
> +
> +async function spawnProcess(name, cmdArgs, processData) {
> +  const opts = {};
> +  opts.command = await Subprocess.pathSearch(name);
> +  opts.arguments = cmdArgs;
> +  opts.environment = {};

You probably don't want this.

::: browser/components/extensions/ext-profiler.js:51
(Diff revision 1)
> +
> +  await readAllData(proc.stdout, processData);
> +}
> +
> +async function getSymbolsFromNM(path) {
> +  const parser = new NMParser();

`NMParser` doesn't appear to exist?

::: browser/components/extensions/ext-profiler.js:61
(Diff revision 1)
> +}
> +
> +function pathComponentsForSymbolFile(debugName, breakpadId) {
> +  let symName = debugName;
> +  if (debugName.endsWith('.pdb')) {
> +    symName = debugName.substr(0, debugName.length - 4);

Either `symName.slice(0, -4)` or `symName = debugName.replace(/(\.pdb)?$/, ".sym")`

::: browser/components/extensions/ext-profiler.js:63
(Diff revision 1)
> +function pathComponentsForSymbolFile(debugName, breakpadId) {
> +  let symName = debugName;
> +  if (debugName.endsWith('.pdb')) {
> +    symName = debugName.substr(0, debugName.length - 4);
> +  }
> +  return [debugName, breakpadId, symName + '.sym'];

Please use template strings rather than concatenation.

::: browser/components/extensions/ext-profiler.js:91
(Diff revision 1)
> +  if (!response.ok) {
> +    throw new Error(`got error status ${response.status}`);
> +  }
> +
> +  const arrayBuffer = await response.arrayBuffer();
> +  return new Uint8Array(arrayBuffer);

Please stick to an ArrayBuffer. Typed arrays can only be cloned to workers. ArrayBuffers can be transferred.

::: browser/components/extensions/ext-profiler.js:118
(Diff revision 1)
> +  // at /path/to/objdir/dist/crashreporter-symbols/.
> +  const objDirDist = getContainingObjdirDist(binaryPath);
> +  if (!objDirDist) {
> +    return null;
> +  }
> +  return OS.Path.join(...[

No need for the `...[`. It's basically a no-op.

::: browser/components/extensions/ext-profiler.js:123
(Diff revision 1)
> +function getSymbolsFromExistingDumpInObjDir(path, debugName, breakpadId) {
> +  return Promise.resolve().then(() => {

Please just make this an async function and skip the `Promise.resolve().then(...)`

::: browser/components/extensions/ext-profiler.js:130
(Diff revision 1)
> +    const symFilePath = filePathForSymFileInObjDir(path, debugName, breakpadId);
> +    if (symFilePath === null) {
> +      throw new Error(`Didn't detect whether ${debugName} ${breakpadId} is in an objdir (path: ${path})`);
> +    }
> +
> +    return OS.File.read(symFilePath);

Can we use a ChromeWorker and have it do the OS.File.read directly rather than passing the data from OSFileWorker->MainThread->SymbolWorker?

::: browser/components/extensions/ext-profiler.js:151
(Diff revision 1)
> +async function getSymbols(debugName, breakpadId) {
> +  const cachedInfo = symbolCache.get(urlForSymFile(debugName, breakpadId));
> +  let path, platform, arch;
> +  if (cachedInfo) {
> +    path = cachedInfo.path;
> +    platform = cachedInfo.platform.toLowerCase();

Can we just lower-case this before we store it in the cache?

::: browser/components/extensions/ext-profiler.js:179
(Diff revision 1)
> +  // usually too large (> 1GB).
> +  const haveAbsolutePath = path && OS.Path.split(path).absolute;
> +  if (haveAbsolutePath) {
> +    try {
> +      const symbolDump = await getSymbolsFromExistingDumpInObjDir(path, debugName, breakpadId);
> +      return await parseSym(symbolDump);

Nit: Never `return await`. Just `return parseSym(...)`

::: browser/components/extensions/ext-profiler.js:190
(Diff revision 1)
> +      const parsed = await parseSym(symbolDump);
> +      return parsed;

Same here. Never `return await`

::: browser/components/extensions/ext-profiler.js:205
(Diff revision 1)
> +  }
> +
> +  // (3) Use `nm` to obtain symbols.
> +  if (platform === 'linux') {
> +    try {
> +      return await getSymbolsFromNM(path);

No `return await`

::: browser/components/extensions/ext-profiler.js:268
(Diff revision 1)
> +  }
> +};
> +
> +this.profiler = class extends ExtensionAPI {
> +  getAPI(context) {
> +    const { extension } = this;

No spaces inside curly braces. Same elsewhere. ESLint won't accept this.

::: browser/components/extensions/ext-profiler.js:270
(Diff revision 1)
> +    function addObserver(fire) {
> +      isRunningObserver.addObserver(fire.async);
> +      return () => {
> +        isRunningObserver.removeObserver(fire.async);
> +      }
> +    }
> +
> +    const runningEventManager = new SingletonEventManager(context,
> +                                                          'profiler.onRunning',
> +                                                          addObserver);

Please do all of this inline in the API object.

::: browser/components/extensions/ext-profiler.js:318
(Diff revision 1)
> +              "You need to start the profiler before you can capture a profile.");
> +          }
> +
> +          const promises = [
> +            gProfiler.getProfileDataAsync(),
> +            primeSymbolStore(gProfiler.sharedLibraries),

Do we really want to do this every time we call getProfile()? Also, why does `primeSymbolStore` need to return a promise? It doesn't do anything async.

::: browser/components/extensions/ext-profiler.js:321
(Diff revision 1)
> +          const promises = [
> +            gProfiler.getProfileDataAsync(),
> +            primeSymbolStore(gProfiler.sharedLibraries),
> +          ];
> +
> +          const [ profile ] = await Promise.all(promises);

No spaces inside square brackets, please.

::: browser/components/extensions/ext-profiler.js:326
(Diff revision 1)
> +          const [ profile ] = await Promise.all(promises);
> +          return profile;
> +        },
> +
> +        async getSymbols(debugName, breakpadId) {
> +          return await getSymbols(debugName, breakpadId);

No `return await`.

::: browser/components/extensions/schemas/profiler.json:34
(Diff revision 1)
> +            "name": "settings",
> +            "type": "object",
> +            "properties": {
> +              "bufferSize": {
> +                "type": "integer",
> +                "description": "The size of the buffer used to store profiling data. A larger value allows capturing a profile that covers a greater amount of time."

Buffer size in what unit?

Also, this should have something like `"minimum": 0`

::: browser/components/extensions/schemas/profiler.json:38
(Diff revision 1)
> +                "type": "integer",
> +                "description": "The size of the buffer used to store profiling data. A larger value allows capturing a profile that covers a greater amount of time."
> +              },
> +              "interval": {
> +                "type": "number",
> +                "description": "Interval between samples of profiling data. A smaller value will increase the detail of the profiles captured."

The interval in milliseconds? Please specify.

::: browser/components/extensions/schemas/profiler.json:44
(Diff revision 1)
> +              },
> +              "features": {
> +                "type": "array",
> +                "description": "A list of active features for the profiler.",
> +                "items": {
> +                  "type": "string"

This should probably be an enum of the features that are available.

::: browser/components/extensions/schemas/profiler.json:47
(Diff revision 1)
> +                "description": "A list of active features for the profiler.",
> +                "items": {
> +                  "type": "string"
> +                }
> +              },
> +              "threads": {

This should probably be optional, rather than accepting a zero-length array.

::: browser/components/extensions/schemas/profiler.json:49
(Diff revision 1)
> +                  "type": "string"
> +                }
> +              },
> +              "threads": {
> +                "type": "array",
> +                "description": "A list of thread names for whic to capture profiles.",

*which

::: browser/components/extensions/schemas/profiler.json:56
(Diff revision 1)
> +        {
> +          "type": "integer",
> +          "name": "panelID",
> +          "description": "For remote profiling, this identifies the WebIDE panel attached to a remote tab.",
> +          "optional": true
> +        }
> +      ]

Nit: Missing an indentation level.

::: browser/components/extensions/schemas/profiler.json:138
(Diff revision 1)
> +      {
> +        "name": "registerDevtoolsPanel",
> +        "type": "function",
> +        "description": "Registers a WebIDE panel which can act as a UI for profiling remote tabs.",
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "type": "string",
> +            "name": "id"
> +          },
> +          {
> +            "type": "object",
> +            "name": "options",
> +            "properties": {
> +              "icon": {
> +                "type": "string",
> +                "description": ""
> +              },
> +              "url": {
> +                "type": "string",
> +                "description": ""
> +              },
> +              "label": {
> +                "type": "string",
> +                "description": ""
> +              },
> +              "tooltip": {
> +                "type": "string",
> +                "description": ""
> +              }
> +            }
> +          }
> +        ]
> +      },
> +      {
> +        "name": "sendDevtoolsPanelMessage",
> +        "type": "function",
> +        "description": "Sends a message to the WebIDE panel identified by panelID.",
> +        "async": "callback",
> +        "parameters": [
> +          {
> +            "type": "number",
> +            "name": "panelID"
> +          },
> +          {
> +            "type": "any",
> +            "name": "message"
> +          }
> +        ]
> +      }

These aren't used.

::: browser/components/extensions/schemas/profiler.json:202
(Diff revision 1)
> +      {
> +        "name": "onDevtoolsPanelMessage",
> +        "type": "function",
> +        "description": "Fires when the WebIDE panel sends a message.",
> +        "parameters": [
> +          {
> +            "name": "message",
> +            "type": "any",
> +            "description": ""
> +          },
> +          {
> +            "name": "panelID",
> +            "type": "number",
> +            "description": ""
> +          },
> +          {
> +            "name": "sendResponse",
> +            "type": "function",
> +            "description": ""
> +          }
> +        ]
> +      }

This isn't used.

::: browser/components/extensions/test/xpcshell/test_ext_profiler_control.js:92
(Diff revision 1)
> +  extension.sendMessage("stop");
> +  await extension.awaitMessage("stopped");
> +
> +  extension.sendMessage("remove runningListener");
> +  await extension.awaitMessage("removed runningListener");
> +  

Nit: Remobe whitespace.

::: toolkit/components/extensions/ExtensionCommon.jsm:1065
(Diff revision 1)
>      let global = Cu.Sandbox(Services.scriptSecurityManager.getSystemPrincipal(), {
>        wantXrays: false,
>        sandboxName: `Namespace of ext-*.js scripts for ${this.processType}`,
>      });
>  
> -    Object.assign(global, {global, Cc, Ci, Cu, Cr, XPCOMUtils, extensions: this});
> +    Object.assign(global, {global, Cc, Ci, Cu, Cr, XPCOMUtils, Worker, extensions: this});

You should be able to get this via `importGlobalProperties`
Attachment #8858343 - Flags: review?(kmaglione+bmo) → review-
Wrapping up my fixup right now but regarding this:

(In reply to Kris Maglione [:kmag] from comment #37)
> ::: toolkit/components/extensions/ExtensionCommon.jsm:1065
> (Diff revision 1)
> >      let global = Cu.Sandbox(Services.scriptSecurityManager.getSystemPrincipal(), {
> >        wantXrays: false,
> >        sandboxName: `Namespace of ext-*.js scripts for ${this.processType}`,
> >      });
> >  
> > -    Object.assign(global, {global, Cc, Ci, Cu, Cr, XPCOMUtils, extensions: this});
> > +    Object.assign(global, {global, Cc, Ci, Cu, Cr, XPCOMUtils, Worker, extensions: this});
> 
> You should be able to get this via `importGlobalProperties`

Neither ChromeWorker nor Worker are currently included in importGlobalProperties right now (http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/js/xpconnect/src/Sandbox.cpp#884), but I can add them in if you think that would be better than injecting them from ExtensionCommon. Would that be preferable?
Flags: needinfo?(kmaglione+bmo)
(In reply to Doug Thayer [:dthayer] from comment #38)
> Neither ChromeWorker nor Worker are currently included in
> importGlobalProperties right now
> (http://searchfox.org/mozilla-central/rev/
> a7334b2896ed720fba25800e11e24952e6037d77/js/xpconnect/src/Sandbox.cpp#884),
> but I can add them in if you think that would be better than injecting them
> from ExtensionCommon. Would that be preferable?

Hm. No, I guess this is fine for now.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8858343 [details]
Bug 1326572 - Provide an API for nsIProfiler

https://reviewboard.mozilla.org/r/130296/#review133468

::: commit-message-3243c:13
(Diff revision 1)
> +- Figure out a faster way to send the large volume of data from
> +  getSymbols all the way from our extension down to the content
> +  process and then into the page's context.

This has come up before. I think it might be a good idea to add functions to ThreadSafeChromeUtils along the lines of `structuredCloneSerialize` and `structuredCloneDeserialize` that operate on ArrayBuffers. Then we can transfer the buffers directly between scopes without much overhead, and deserialize directly into the final extension context when we're done.

That can definitely be done as a follow-up, though.
(In reply to Kris Maglione [:kmag] from comment #40)
> This has come up before. I think it might be a good idea to add functions to
> ThreadSafeChromeUtils along the lines of `structuredCloneSerialize` and
> `structuredCloneDeserialize` that operate on ArrayBuffers. Then we can
> transfer the buffers directly between scopes without much overhead, and
> deserialize directly into the final extension context when we're done.
> 
> That can definitely be done as a follow-up, though.

Yeah - there's code in the old extension to turn our address -> symbol Map into an ArrayBuffer, which is actually the final form currently expected by perf-html.io. The problem is, unless I'm mistaken, we need to extend sendMessage in runtime.Port and elsewhere to accept a transfer parameter which it can forward to the underlying message manager. Has this been discussed before?

To clarify a bit, this is the route that I'm hoping to optimize:

ParseSymbols worker  -->  extension       (self.postMessage - OK)
extension            -->  content script  (port.sendMessage - not OK: need transferable support)
content script       -->  page            (win.postMessage - OK)

With this in mind, structuredCloneDeserialize wouldn't get us very far, because we'd need access to it on perf-html.io. Did this make sense or did I not understand your initial point correctly?
(In reply to Doug Thayer [:dthayer] from comment #41)
> extension            -->  content script  (port.sendMessage - not OK: need
> transferable support)

This crosses a process boundary, so Gecko's current implementation will ignore the transfer parameter and just copy anyway. In order to avoid the cross-process copy, you'd need to put the buffer contents into cross-process shared memory, and we don't do that at the moment. (And unless you allocate the array buffer contents into a shared memory region from the start, it wouldn't even avoid the copy, if I understand things correctly.)

> content script       -->  page            (win.postMessage - OK)

With the SDK-based add-on there was another copy at this step, because we were using cloneInto to make the array accessible to the page script, and there is no transferInto equivalent that avoids the copy. So if WebExtensions are avoiding that problem, then there's already an improvement here. Nice!
(In reply to Doug Thayer [:dthayer] from comment #41)
> ParseSymbols worker  -->  extension       (self.postMessage - OK)
> extension            -->  content script  (port.sendMessage - not OK: need
> transferable support)
> content script       -->  page            (win.postMessage - OK)

Hm. Interesting. Well, the content script and background page are generally
not in the same process, so I'm not sure how much we can improve that
situation, other than giving the content script direct access to this API
(which we probably don't want).

But we could probably have the background script upload directly to perfht.ml,
if an upload is happening. Or we might be able to do something clever with
Blob URLs...

Anyway, I don't know enough about the underlying architecture to comment
intelligently on this, I think.

> With this in mind, structuredCloneDeserialize wouldn't get us very far,
> because we'd need access to it on perf-html.io. Did this make sense or did I
> not understand your initial point correctly?

OK, in that case we'd probably just want to convert the data to that format in
the worker, and skip the structured clone part. We might need some new helper
code to transfer the ownership of the ArrayBuffer's contents to the
extension's scope, though, rather than copying or proxying it. We'd have to do
the same for the messaging.

I guess we could add an option to cloneInto to transfer ArrayBuffers rather
than cloning them.
(In reply to Markus Stange [:mstange] from comment #42)
> In order to avoid the cross-process copy, you'd need to put the buffer
> contents into cross-process shared memory, and we don't do that at the
> moment. (And unless you allocate the array buffer contents into a shared
> memory region from the start, it wouldn't even avoid the copy, if I
> understand things correctly.)

Hm. I wonder if we could somehow make this work with a SharedArrayBuffer... In
theory those should be able to be transferred between processes, and between
chrome and content scopes... I've still been thinking about them as highly
experimental, but so is this API, I guess, so...
Comment on attachment 8858343 [details]
Bug 1326572 - Provide an API for nsIProfiler

https://reviewboard.mozilla.org/r/130296/#review133022

> Do we really want to spawn a new worker each time we call this? Either way, please explicitly close the worker when you're done with it.

I think so. This will only happen a handful of times, so keeping a worker around for it seems unnecessary. Added a close() in the worker though.

> Can we use a ChromeWorker and have it do the OS.File.read directly rather than passing the data from OSFileWorker->MainThread->SymbolWorker?

Additionally moved the fetch call to the worker.

> Nit: Never `return await`. Just `return parseSym(...)`

There are a few unnecessary return awaits here, but return await has the difference that exceptions can be caught. Added a comment for this though. But let me know if you'd like something else.
Comment on attachment 8858343 [details]
Bug 1326572 - Provide an API for nsIProfiler

https://reviewboard.mozilla.org/r/130296/#review135026

This is some great stuff. I really like what you did for testing!
Attachment #8858343 - Flags: review?(mstange) → review+
mstange, I'm struggling a bit with how to call dump_syms as native code. The DumpSymbols class ends up using RTTI and makes heavy use of STL types. Thoughts?
Flags: needinfo?(mstange)
(In reply to Doug Thayer [:dthayer] from comment #48)
> mstange, I'm struggling a bit with how to call dump_syms as native code. The
> DumpSymbols class ends up using RTTI and makes heavy use of STL types.
> Thoughts?

I think we're technically allowed to compile and use modules with RTTI and STL types, as long as we never allow exceptions to propagate through ordinary gecko code, or give it access to external objects.

But perhaps we should just stick to Emscripten, and use a WASM worker for now. We already do that for the language detection library (minus the WASM part), and it hasn't caused any issues.

Also, is the source for the dump_symbols routine available anywhere?
Comment on attachment 8858343 [details]
Bug 1326572 - Provide an API for nsIProfiler

https://reviewboard.mozilla.org/r/130296/#review135442

Looks good. Thanks!

::: browser/app/profile/firefox.js:74
(Diff revision 2)
> +#ifdef XP_LINUX
> +pref("extensions.geckoProfiler.getSymbolRules", "localBreakpad,remoteBreakpad,nm,dumpSyms");
> +#else
> +#ifdef XP_MACOSX
> +pref("extensions.geckoProfiler.getSymbolRules", "localBreakpad,remoteBreakpad,dumpSyms");
> +#else
> +pref("extensions.geckoProfiler.getSymbolRules", "localBreakpad,remoteBreakpad");
> +#endif

Nit:

    #if defined(XP_LINUX)
    ...
    #elif defined(XP_MACOSX)
    ...
    #else // defined(XP_WIN)
    ...
    #endif

::: browser/components/extensions/ParseSymbols-worker.js:4
(Diff revision 2)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +// eslint-disable-next-line no-unused-vars
> +/* global importScripts, fetch, OS, postMessage, close, ParseSymbols, onmessage:true */

Nit:

    /* eslint-env worker */
    /* globals OS, ParseSymbols */

::: browser/components/extensions/ParseSymbols-worker.js:54
(Diff revision 2)
> +    } else if (e.data.url) {
> +      text = await fetchSymbolFile(e.data.url);
> +    }
> +
> +    const result = parse(text);
> +    postMessage({result}, result.map(r => r.buffer));

Hm. Does this actually work? I can see how it might, but looking at the StructuredClone code, I'm not entirely sure.

::: browser/components/extensions/ParseSymbols.jsm:16
(Diff revision 2)
> +    while (pos + encodedString.length > buffer.length) {
> +      let newBuffer = new Uint8Array(buffer.length << 1);

I know this isn't likely to happen much in practice, but it's not very efficient to repeatedly create new buffers until we have one that fits. We could repeatedly left shift until we have a size that's big enough, without creating buffers, but it would be nice to just calculate the new size with something like:

    let size = pos + encodedString.length;
    if (size < buffer.length) {
      size = 2 << Math.log(size) / Math.log(2);
      let newBuffer = new Uint8Array(size);
      newBuffer.set(buffer);
      buffer = newBuffer;
    }

::: browser/components/extensions/ext-browser.js:187
(Diff revision 2)
>    },
> +  geckoProfiler: {
> +    url: "chrome://browser/content/ext-geckoProfiler.js",
> +    schema: "chrome://browser/content/schemas/geckoProfiler.json",
> +    scopes: ["addon_parent"],
> +    manifest: ["geckoProfiler"],

This isn't necessary. It's for APIs that need to be loaded and initialized early when an extension has a particular top-level manifest entry.

::: browser/components/extensions/ext-geckoProfiler.js:8
(Diff revision 2)
> +XPCOMUtils.defineLazyModuleGetter(this, "Subprocess", "resource://gre/modules/Subprocess.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ParseSymbols", "resource:///modules/ParseSymbols.jsm");
> +XPCOMUtils.defineLazyGetter(this, "OS", () => Cu.import("resource://gre/modules/osfile.jsm", {}).OS);

Nit: Please keep sorted.

::: browser/components/extensions/ext-geckoProfiler.js:10
(Diff revision 2)
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.importGlobalProperties(["fetch"]);
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Subprocess", "resource://gre/modules/Subprocess.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ParseSymbols", "resource:///modules/ParseSymbols.jsm");
> +XPCOMUtils.defineLazyGetter(this, "OS", () => Cu.import("resource://gre/modules/osfile.jsm", {}).OS);

XPCOMUtils.defineLazyModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");

::: browser/components/extensions/ext-geckoProfiler.js:83
(Diff revision 2)
> +    // The letter has a meaning, but we ignore it.
> +
> +    const regex = /([^ ]+) (?:. )?(.*)/;
> +    let match = regex.exec(line);
> +    if (match) {
> +      const [address, symbol] = match.slice(1);

Nit: `const [, address, symbol] = match;`

::: browser/components/extensions/ext-geckoProfiler.js:101
(Diff revision 2)
> +    // Setting environment to {} will prevent Subprocess.jsm from copying our environment
> +    // over, which we don't need it to do.
> +    environment: {},

Is there any particular reason we want the child process to startup up with no environment? A lot of programs don't respond well to that, especially on Windows.

::: browser/components/extensions/ext-geckoProfiler.js:216
(Diff revision 2)
> +
> +this.geckoProfiler = class extends ExtensionAPI {
> +  getAPI(context) {
> +    const extensionWhitelist = Services.prefs.getCharPref(PREF_ACCEPTED_EXTENSION_IDS).split(",");
> +    if (!extensionWhitelist.includes(context.extension.id)) {
> +      throw new Error("Only whitelisted extensions are allowed to access the geckoProfiler.");

This isn't a good place to throw an error. We should check this in Extension.jsm when processing the manifest permissions list, instead. If you call `extension.manifestError` at that point, it will prevent installation of the invalid add-on.

::: browser/components/extensions/ext-geckoProfiler.js:312
(Diff revision 2)
> +                  // symbolDump = await dumpSyms(path, platform, arch);
> +                  // return parseSym(symbolDump);
> +                  break;
> +              }
> +            } catch (e) {
> +            }

Please add a comment here about why the exception is being ignored. Our ESLint rules require it.

::: browser/components/extensions/test/xpcshell/test_ext_geckoProfiler_control.js:76
(Diff revision 2)
> +  });
> +};
> +
> +add_task(async function testProfilerControl() {
> +  const acceptedExtensionIdsPref = "extensions.geckoProfiler.acceptedExtensionIds";
> +  const oldAcceptedExtensionIds = Services.prefs.getCharPref(acceptedExtensionIdsPref);

Nit: No need for this. Just clearUserPref when you're done.
Attachment #8858343 - Flags: review?(kmaglione+bmo) → review+
(In reply to Doug Thayer [:dthayer] from comment #48)
> mstange, I'm struggling a bit with how to call dump_syms as native code. The
> DumpSymbols class ends up using RTTI and makes heavy use of STL types.
> Thoughts?

Maybe we should discard the idea of using dump_syms for now, and try to use getSymbolsFromNM on Mac. The only place where dump_syms is used atm is when you profile a local build on Mac; and if you do that, you have Xcode installed, which has installed nm.
Flags: needinfo?(mstange)
Comment on attachment 8858343 [details]
Bug 1326572 - Provide an API for nsIProfiler

https://reviewboard.mozilla.org/r/130296/#review135442

> Hm. Does this actually work? I can see how it might, but looking at the StructuredClone code, I'm not entirely sure.

It does. Validated by writing a simple script that bounced a large typed array back and forth a few times and comparing with and without the transfer list.

> Is there any particular reason we want the child process to startup up with no environment? A lot of programs don't respond well to that, especially on Windows.

Coming back to it I'm not sure why I was so zealous about doing this. The old add-on used child_process from the SDK, and didn't supply an env, which led to it eventually passing a {} to Subprocess.jsm, so I wrote this to keep the behavior identical, without thinking that it probably didn't matter. Removed and everything still works just fine.
kmag, would you mind taking a quick look again? Specifically at the change to Extension.jsm to check the add-on ID, and the change to getSymbolsFromNM to get it to work on Mac?
(forgot NI)
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8858343 [details]
Bug 1326572 - Provide an API for nsIProfiler

https://reviewboard.mozilla.org/r/130296/#review136548

::: browser/components/extensions/ext-geckoProfiler.js:153
(Diff revisions 2 - 3)
> +
> +  await spawnProcess("nm", args, data => parser.consume(data));
> +  await spawnProcess("nm", ["-D", ...args], data => parser.consume(data));
> +  let {syms, approximateLength} = parser.finish();
> +
> +  if (Services.appinfo.OS == 'Darwin') {

Nit: `===`

::: browser/components/extensions/ext-geckoProfiler.js:160
(Diff revisions 2 - 3)
> +    const values = keys.map(k => syms.get(k));
> +    const demangler = new CppFiltParser(keys.length);
> +    await spawnProcess("c++filt", [], data => demangler.consume(data), values.join("\n"));
> +    const newSymbols = demangler.finish();
> +    approximateLength = 0;
> +    for (let i = 0; i < newSymbols.length; i++) {

Nit: `for (let [i, symbol] of newSymbols.entries())`

::: browser/components/extensions/ext-geckoProfiler.js:143
(Diff revision 3)
> +
> +async function getSymbolsFromNM(path) {
> +  const parser = new NMParser();
> +
> +  const args = [path];
> +  if (Services.appinfo.OS != 'Darwin') {

Nit: `!==`

::: toolkit/components/extensions/Extension.jsm:522
(Diff revision 3)
>      for (let perm of this.manifest.permissions) {
>        if (perm == "contextualIdentities" && !Preferences.get("privacy.userContext.enabled")) {
>          continue;
>        }
>  
> +      if (perm == "geckoProfiler") {

Nit: Please use strict equality (`===` and `!==`) in WebExtension code.

::: toolkit/components/extensions/Extension.jsm:524
(Diff revision 3)
>          continue;
>        }
>  
> +      if (perm == "geckoProfiler") {
> +        const acceptedExtensions = Preferences.get("extensions.geckoProfiler.acceptedExtensionIds");
> +        if (!acceptedExtensions.includes(this.id)) {

I think this needs to be something like `acceptedExtensions.split(",").includes(...)`, or we'll wind up allowing extensions with IDs like "r@moz" to access the API.

::: toolkit/components/extensions/Extension.jsm:526
(Diff revision 3)
>  
> +      if (perm == "geckoProfiler") {
> +        const acceptedExtensions = Preferences.get("extensions.geckoProfiler.acceptedExtensionIds");
> +        if (!acceptedExtensions.includes(this.id)) {
> +          this.manifestError("Only whitelisted extensions are allowed to access the geckoProfiler.");
> +        }

Please also `continue`, just to be safe.
kmag would you mind landing this for me? (or anyone else)
I can't land them until the review issues have been marked as resolved.

Also, in the future, you can just add the checkin-needed keyword and a sheriff will check things in for you.
Flags: needinfo?(kmaglione+bmo)
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/87bba05a5669
Provide an API for nsIProfiler r=kmag,mstange
https://hg.mozilla.org/mozilla-central/rev/87bba05a5669
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
This probably isn't worth documenting at this point. It's only for internal use, and it's subject to change.
Keywords: dev-doc-needed
The WebExtension patches were merged in https://github.com/devtools-html/Gecko-Profiler-Addon/pull/44

Updating the legacy add-on should swap it out for the new WebExtension version (Gecko Profiler 0.4).

Great work on this, folks!
Depends on: 1362785
Depends on: 1362786
Depends on: 1362792
Depends on: 1362861
Depends on: 1365400
Depends on: 1371003
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.