Closed Bug 1362800 Opened 4 years ago Closed 4 years ago

Add nsIProfiler.getProfileAsArrayBuffer()

Categories

(Core :: Gecko Profiler, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: mstange)

Details

Attachments

(2 files, 1 obsolete file)

The profile is captured on the parent process main thread, but we don't need to access its contents in the parent process. Only perf.html, running in the content process, actually needs the data.

In the meantime, having JS objects for the contents of the profile in the parent process causes performance problems due to multiple copies, Xrays, and GC pressure.

We can avoid most of those problems by keeping the profile as an opaque buffer until we need its contents. Adding an API to obtain the profile as a typed array would make that possible.
Comment on attachment 8866075 [details]
Bug 1362800 - Expose geckoProfiler.getProfileAsArrayBuffer.

https://reviewboard.mozilla.org/r/137678/#review140792

It seems a bit weird to have an ArrayBuffer that just contains a JSON string, but I guess it makes sense, all things considered.

::: browser/components/extensions/ext-geckoProfiler.js:307
(Diff revision 1)
>            return Services.profiler.getProfileDataAsync();
>          },
>  
> +        async getProfileAsArrayBuffer() {
> +          if (!Services.profiler.IsActive()) {
> +            throw new Error("The profiler is stopped. " +

This should be `ExtensionError`, or the extension exposed to the extension code will only say "An unexpected error occurred". The same goes for the error above. Sorry for not catching this before.
Attachment #8866075 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8866074 [details]
Bug 1362800 - Add a way to get the profile as an array buffer.

https://reviewboard.mozilla.org/r/137676/#review140804

::: tools/profiler/gecko/nsProfiler.cpp:298
(Diff revision 1)
>    promise.forget(aPromise);
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsProfiler::GetProfileDataAsArrayBuffer(double aSinceTime, JSContext* aCx,

Does |implicit_jscontext| not put the JSContext argument first? Huh.

::: tools/profiler/gecko/nsProfiler.cpp:311
(Diff revision 1)
> +
> +  if (NS_WARN_IF(!aCx)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsIGlobalObject* go = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));

I find |go| very hard to read as "global object"! And I don't recall seeing that name used before. Please change to |obj| or |globalObj| or similar.

::: tools/profiler/gecko/nsProfiler.cpp:324
(Diff revision 1)
> +  if (NS_WARN_IF(result.Failed())) {
> +    return result.StealNSResult();
> +  }
> +
> +  mGatherer->Start(aSinceTime)->Then(
> +    AbstractThread::MainThread(), __func__,

Is __func__ supported on all profiler platforms?
Attachment #8866074 - Flags: review?(n.nethercote) → review+
Comment on attachment 8866074 [details]
Bug 1362800 - Add a way to get the profile as an array buffer.

https://reviewboard.mozilla.org/r/137676/#review140804

> Does |implicit_jscontext| not put the JSContext argument first? Huh.

... no idea, but this works, and it's what GetProfileDataAsync does.

> I find |go| very hard to read as "global object"! And I don't recall seeing that name used before. Please change to |obj| or |globalObj| or similar.

Will do. I'll also change GetProfileDataAsync because that's where I copied this from.

> Is __func__ supported on all profiler platforms?

I assume so, since I cargo-culted it from media code, e.g. http://searchfox.org/mozilla-central/source/dom/media/MediaDecoderReaderWrapper.cpp#54
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/785cf0c4b67b
Add a way to get the profile as an array buffer. r=njn
https://hg.mozilla.org/integration/autoland/rev/0492e6f16df1
Expose geckoProfiler.getProfileAsArrayBuffer. r=kmag
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/autoland/rev/19c8b67843b4
Add a way to get the profile as an array buffer. r=njn
https://hg.mozilla.org/integration/autoland/rev/36be81d75d35
Expose geckoProfiler.getProfileAsArrayBuffer. r=kmag
https://hg.mozilla.org/mozilla-central/rev/19c8b67843b4
https://hg.mozilla.org/mozilla-central/rev/36be81d75d35
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Attached file profile-failure.json (obsolete) —
This is an example of profile I get from gecko. I think the first line has been cut by the process I used to retrieve it (copy() from devtools), but we clearly see an issue in the second line.

I get this error when trying to retrieve it:
SyntaxError: JSON.parse: expected double-quoted property name at line 2 column 16 of the JSON data.
OK, Markus told me the error we get likely comes from elsewhere.
Attachment #8867201 - Attachment is obsolete: true
Flags: needinfo?(mstange)
You need to log in before you can comment on or make changes to this bug.