1362786 - getSymbolsFromNM should run on a worker thread

Status: RESOLVED FIXED

(WebExtensions :: Experiments, defect, P1)

Description

[Markus Stange [:mstange]]

It's creating a lot of traffic on the main thread, and about 300ms of jank at the end for libxul. See this profile: https://perfht.ml/2qbvEAd

Comment 1

[Kris Maglione [:kmag]]

I'd initially planned to make a version of the Subprocess.jsm API available to workers, but I never got around to it. It should probably work well enough to transfer the ArrayBuffers from the main thread to a worker and do the processing there, though.

Although, it also looks like pathSearch is surprisingly expensive, largely just from creating the environment object. We should probably do that once, rather than once for every nm call.

Comment 2

[Markus Stange [:mstange]]

Would this proposal still cause us to run code on the main thread for every chunk that we read? It would be good to avoid that, even just to reduce GC pressure for the main thread.
The period of jank that I mentioned in comment 0 was actually 400ms long, the last 100ms being garbage collection.

Comment 3

[Kris Maglione [:kmag]]

Yes, but they could be quite big chunks. And I don't think GC would be a major issue, since we'd be transferring the ArrayBuffers to the worker thread, and wouldn't be creating very much main thread garbage in the process.

Comment 4

[Markus Stange [:mstange]]

Ok, let's give it a try then.

Comment 5

[:shell escalante]

should kris or john thayer (sp?) take this?

Comment 6

[Andy McKay]

Is this something run when the gecko profiler is run, or all the time?

Comment 7

[Markus Stange [:mstange]]

The former.

Comment 8

[Andy McKay]

Would you be able to look at this Doug?

Comment 9

[Markus Stange [:mstange]]

Tangentially related: getSymbolsFromNM also seems a lot slower than it needs to be, at least for libxul. Last time I looked at a profile for this, the cost mostly seemed to be in the structured cloning of the chunks. I think increasing the size of the chunks (so that there are fewer of them) might already help with the speed. If I run "nm .../XUL > somefile", it takes less than half a second on my machine, so I think there's a lot of room for improvement here.

Comment 10

[Alex Thayer [:alexical] (she/her)]

(In reply to Andy McKay [:andym] from comment #8)
> Would you be able to look at this Doug?

Yeah I can take this.

Comment 11

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1362786 - (1) Increase subprocess BUFFER_SIZE

While 4KiB is a decently sized buffer, bumping it to 32 on my
machine reduced the time for getSymbolsFromNM from 12s to 8s. If
32 is too large for the typical use of this, then we can instead
just parameterize this so that we can specify as large a buffer
as we want for our use case.

Review commit: https://reviewboard.mozilla.org/r/159946/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159946/

Comment 12

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1362786 - (2) Run NMParser in worker

Pulls out the NMParser work (parsing nm results and turning
them into an ArrayBuffer'd map of addresses to symbols) into
a worker.

For OSX we will still need to do some work to run c++filt in the
background, but this gets us most of the way there. Without a
Subprocess.jsm usable from a worker, we'll have to bounce data
back to the main thread in order to bounce it to the c++filt
worker.

Review commit: https://reviewboard.mozilla.org/r/159948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159948/

Comment 13

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1362786 - (3) Run CppFiltParser in worker

Pulls out the CppFiltParser work into a worker to run in the
background and avoid the jank induced by the final
convertSymsMapToExpectedSymFormat call.

Profiling shows that after this change there is virtually no
jank when asking the geckoProfiler API for symbols.

Review commit: https://reviewboard.mozilla.org/r/159950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159950/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8888920 [details]
Bug 1362786 - (1) Increase subprocess BUFFER_SIZE

https://reviewboard.mozilla.org/r/159946/#review167008

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8888921 [details]
Bug 1362786 - (2) Run NMParser in worker

https://reviewboard.mozilla.org/r/159948/#review167022

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8888922 [details]
Bug 1362786 - (3) Run CppFiltParser in worker

https://reviewboard.mozilla.org/r/159950/#review167024

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/242a12eae20e
(1) Increase subprocess BUFFER_SIZE r=kmag
https://hg.mozilla.org/integration/autoland/rev/71b419e1788d
(2) Run NMParser in worker r=kmag
https://hg.mozilla.org/integration/autoland/rev/c1a15605e8a2
(3) Run CppFiltParser in worker r=kmag

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/242a12eae20e
https://hg.mozilla.org/mozilla-central/rev/71b419e1788d
https://hg.mozilla.org/mozilla-central/rev/c1a15605e8a2