If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

getSymbolsFromNM should run on a worker thread

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Experiments
P1
normal
RESOLVED FIXED
5 months ago
2 months ago

People

(Reporter: mstange, Assigned: dthayer)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

5 months ago
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
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.
See Also: → bug 1362788
(Reporter)

Comment 2

5 months ago
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.
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.
(Reporter)

Comment 4

5 months ago
Ok, let's give it a try then.

Comment 5

4 months ago
should kris or john thayer (sp?) take this?
Flags: needinfo?(amckay)
Priority: -- → P1
Whiteboard: triaged

Comment 6

4 months ago
Is this something run when the gecko profiler is run, or all the time?
Flags: needinfo?(amckay)
(Reporter)

Comment 7

4 months ago
The former.

Comment 8

2 months ago
Would you be able to look at this Doug?
Flags: needinfo?(dothayer)
(Reporter)

Comment 9

2 months ago
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.
(Assignee)

Comment 10

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

Yeah I can take this.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Flags: needinfo?(dothayer)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8888920 [details]
Bug 1362786 - (1) Increase subprocess BUFFER_SIZE

https://reviewboard.mozilla.org/r/159946/#review167008
Attachment #8888920 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8888921 [details]
Bug 1362786 - (2) Run NMParser in worker

https://reviewboard.mozilla.org/r/159948/#review167022
Attachment #8888921 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8888922 [details]
Bug 1362786 - (3) Run CppFiltParser in worker

https://reviewboard.mozilla.org/r/159950/#review167024
Attachment #8888922 - Flags: review?(kmaglione+bmo) → review+

Comment 17

2 months ago
Pushed by dothayer@mozilla.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/242a12eae20e
https://hg.mozilla.org/mozilla-central/rev/71b419e1788d
https://hg.mozilla.org/mozilla-central/rev/c1a15605e8a2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.