1581963 - Use bulk sending APIs to transmit the profile from the perf actor, when sending the profile from Android to the desktop browser (startBulkSend, startBulkRequest, copyFrom)

Status: RESOLVED FIXED

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

Description

[Greg Tatum [:gregtatum]]

Right now a JSON profile is sent over the RDP. It seems that array buffers are now supports for some instances. We should see if we can make it faster when running locally by getting the gzipped version sent over RDP, rather than the full Profile.

The two browser calls are:

• Services.profiler.getProfileDataAsGzippedArrayBuffer
• Services.profiler.getProfileDataAsync

Comment 1

[Markus Stange [:mstange]]

(In reply to Greg Tatum [:gregtatum] from comment #0)

It seems that array buffers are now supports for some instances.

Nicolas, do you know more details about this? Can array buffers be sent over the devtools protocol these days?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Markus Stange [:mstange] from comment #1)

(In reply to Greg Tatum [:gregtatum] from comment #0)

It seems that array buffers are now supports for some instances.

Nicolas, do you know more details about this? Can array buffers be sent over the devtools protocol these days?

So I don't know much about this, but I don't think we can send ArrayBuffers through RDP.
Looking at the code, it seems that we're interested in this function: https://searchfox.org/mozilla-central/rev/5227b2bd674d49c0eba365a709d3fb341534f361/devtools/shared/performance-new/gecko-profiler-interface.js#147-186

It's in devtools/shared which means it can be accessed from the DevTools client and the DevTools server.
So what could be done is to detect when we're not debugging a remote server, and in such case, directly call getProfileAndStopProfiler from the client (from here I guess https://searchfox.org/mozilla-central/rev/5227b2bd674d49c0eba365a709d3fb341534f361/devtools/client/performance-new/store/actions.js#211)

That's something we thought at some point for screenshots, where a long time is spent stringifying data so we can send them through RDP, although we could directly access them when we're not debugging remotely.

Comment 3

[Markus Stange [:mstange]]

Thanks! It's somewhat unsatisfying to leave remote targets on the slow path, though. And it sounds like the ability to remotely transmit array buffers efficiently would be useful in other places, too. I can file a bug about adding this capability; do you know who'd be the right person to assess the feasibility of this?

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Markus Stange [:mstange] from comment #3)

Thanks! It's somewhat unsatisfying to leave remote targets on the slow path, though. And it sounds like the ability to remotely transmit array buffers efficiently would be useful in other places, too. I can file a bug about adding this capability; do you know who'd be the right person to assess the feasibility of this?

ochameau and jdescottes should know more than me about the different DevTools transport layers

Comment 5

[Alexandre Poirot [:ochameau]]

Yes, there is a binary escape path in RDP.
Unfortunately, it isn't super documented, nor used.

There is this one usage you can take as example, the memory profile generated by the memory panel.

Server side code:
https://searchfox.org/mozilla-central/rev/c0fc8c4852e927b0ae75d893d35772b8c60ee06b/devtools/server/actors/heap-snapshot-file.js#57-82
On this side, the key here is to use this.conn.startBulkSend and bulk.copyFrom.

Client side code:
https://searchfox.org/mozilla-central/rev/c0fc8c4852e927b0ae75d893d35772b8c60ee06b/devtools/client/fronts/memory.js#82-113
Note that this client code should be using DevToolsClient.startBulkRequest. (no idea why it isn't)
On this side, it is about using client.startBulkRequest and const { copyTo } = await request;

This is completely bypassing protocol.js, so there is nothing to declare in your spec (i.e. devtools/shared/spec).

Let me know if you need any more help with this.

Comment 6

[Markus Stange [:mstange]]

Thanks a lot for these pointers! I will give this a try, once I get around to it.

Comment 7

[Markus Stange [:mstange]]

Hah, funnily enough, the profiler is the primary use case in the description of bug 797639, which added the bulk writing capabilities 9 years ago. "Performance profiling" is also mentioned in a section about bulk data in the Remote Debugging Protocol documentation.

Comment 8

[Markus Stange [:mstange]]

Morphing this bug to switch to the bulk sending APIs from comment 5. At the moment, we parse the profile into JSON on the Android device by using the API getProfileDataAsync here, and then we send the JS object across the pipe.

Comment 9

[Julien Wajsberg [:julienw]]

(In reply to Alexandre Poirot [:ochameau] from comment #5)

Client side code:
https://searchfox.org/mozilla-central/rev/c0fc8c4852e927b0ae75d893d35772b8c60ee06b/devtools/client/fronts/memory.js#82-113
Note that this client code should be using DevToolsClient.startBulkRequest. (no idea why it isn't)
On this side, it is about using client.startBulkRequest and const { copyTo } = await request;

I think it's not using startBulkRequest simply because the request itself isn't a "bulk" message, it only needs to send the snapshotId. I believe we have the same requirement to retrieve the profile data: we don't even have anything to send.
So I believe we don't need startBulkRequest either on the request side.

Comment 10

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1581963 - [devtools] Add a new trait to control the use of the bulk API for the performance panel r=canaltinova

This also adds the trait to the Redux state, so that it can be retrieved
easily from anywhere.

(This may not be useful at the end, and I may remove it, but I'm keeping
it here in case it is)

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: Bug 1581963 - [devtools] Implement a new remote function to gather the profile data, using the bulk api r=canaltinova!,#devtools-reviewers!,#devtools-backward-compat-reviewers!

Comment 12

[Julien Wajsberg [:julienw]]

Attachment: Bug 1581963 - [devtools] Add a new trait to control the use of the bulk API for the performance panel r=canaltinova!,#devtools-reviewers!,#devtools-backward-compat-reviewers!

Comment 13

[Julien Wajsberg [:julienw]]

Attachment: Bug 1581963 - [devtools] Make stream-utils support both AsyncStreams and normal streams r=#devtools-reviewers!

Comment 14

[Julien Wajsberg [:julienw]]

Attachment: Bug 1581963 - [devtools] Handle errors properly when capturing a profile r=canaltinova

Comment 15

[Julien Wajsberg [:julienw]]

I tested capturing a profile with the Media preset while playing a video on Youtube on my own phone.
In all cases the new code was capturing the profile faster. As I expected the magnitude of the difference depends on the size of the profile.

Each time I give 2 numbers. The base time is when I click the "capture" button. The first measurement is when the button turns back to blue. The second measurement is when the profile opens in the browser. I think (but it would be good to check) that the first measurement is the capture time on device, while the second part is when the profile data is transfered (the second is is cumulative, so it includes also the capture).

For a 20-second profile:
old: 6s, then 14s
new: 5s, then 9s

For a 30-second profile:
old: 9s, then 20s
new: 5s, then 10s

For a 1-minute profile:
old: 16s then 1:03s
new: 14s then 24s

TBH I expected that the capture time could be longer, because now we're gzipping the profile on-device while previously we weren't doing that. But it looks like that gzipping it provides some improvement too. Or it's possible that it's just the matter of using a plain buffer rather than a JS object when handling the data on the devtools server layer.

Comment 16

[Julien Wajsberg [:julienw]]

Attachment: Bug 1581963 - [profiler] Remove an unused feature from the default list of features r=canaltinova

Comment 17

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1581963 - Change implementation of the bulk copy -- to be folded into another commit once it's ready

Comment 18

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1581963 - Improve DevtoolsUtils logging facility to include a timestamp r=ochameau

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1581963 - [devtools profiler] Add some logs around the copy code r=canaltinova

Comment 20

[Phabricator Automation]

Comment on attachment 9481569 [details]
WIP: Bug 1581963 - Improve DevtoolsUtils logging facility to include a timestamp r=ochameau

Revision D246970 was moved to bug 1963284. Setting attachment 9481569 [details] to obsolete.

Comment 21

[Julien Wajsberg [:julienw]]

Attachment: Bug 1581963 - [devtools] Add copyFromBuffer and copyToBuffer to the bulk packet r=ochameau

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a64e67bffde
[profiler] Remove an unused feature from the default list of features r=canaltinova,devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/6a4878977fca
[devtools] Add a new trait to control the use of the bulk API for the performance panel r=devtools-reviewers,devtools-backward-compat-reviewers,profiler-reviewers,nchevobbe,mstange
https://hg.mozilla.org/integration/autoland/rev/18010dd7c6a3
[devtools] Implement a new remote function to gather the profile data, using the bulk api r=devtools-backward-compat-reviewers,profiler-reviewers,ochameau,mstange
https://hg.mozilla.org/integration/autoland/rev/8e3df78fdb03
[devtools] Handle errors properly when capturing a profile r=profiler-reviewers,mstange
https://hg.mozilla.org/integration/autoland/rev/0d13792947ea
[devtools] Add copyFromBuffer and copyToBuffer to the bulk packet r=ochameau,devtools-reviewers,profiler-reviewers,canaltinova

Comment 23

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/0a64e67bffde
https://hg.mozilla.org/mozilla-central/rev/6a4878977fca
https://hg.mozilla.org/mozilla-central/rev/18010dd7c6a3
https://hg.mozilla.org/mozilla-central/rev/8e3df78fdb03
https://hg.mozilla.org/mozilla-central/rev/0d13792947ea

Comment 24

[Julien Wajsberg [:julienw]]

In addition to the transfer from debuggee to debugger being faster, I think this work also made it so that the transfer from the firefox machinery to the firefox profiler UI is much faster: indeed the data is now an arraybuffer instead of a JS object, and as a result it's a transferable object instead of being structured cloned.

From my testing it seems to be reduced from 15 sec to 5 sec for a ~30MB profile.

The long part is now the symbolication part, esp libxul.so. But this happens also on desktop.

Comment 25

[Julien Wajsberg [:julienw]]

Some numbers from capturing 1 minute with the media preset on youtube.

Before:
23.4 MB (gzipped) according to the text in the upload dialog.

• 12 sec to capture
• 18 sec to transfer
• 13 sec to open the UI

After:
27.6 MB (gzipped) according to the text in the upload dialog.

• 12 sec to capture
• near instant to transfer
• 6 sec to open the UI