Closed
Bug 1362800
Opened 8 years ago
Closed 8 years ago
Add nsIProfiler.getProfileAsArrayBuffer()
Categories
(Core :: Gecko Profiler, enhancement)
Core
Gecko Profiler
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•8 years ago
|
||
mozreview-review |
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 4•8 years ago
|
||
mozreview-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+
Assignee | ||
Comment 5•8 years ago
|
||
mozreview-review-reply |
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
Backed out for eslint failures: https://treeherder.mozilla.org/logviewer.html#?job_id=98165885&repo=autoland
https://hg.mozilla.org/integration/autoland/rev/a9dedd866433
Flags: needinfo?(mstange)
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
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
![]() |
||
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/19c8b67843b4
https://hg.mozilla.org/mozilla-central/rev/36be81d75d35
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 13•8 years ago
|
||
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.
Comment 14•8 years ago
|
||
OK, Markus told me the error we get likely comes from elsewhere.
Updated•8 years ago
|
Attachment #8867201 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(mstange)
You need to log in
before you can comment on or make changes to this bug.
Description
•