Closed Bug 1362786 Opened 2 years ago Closed 2 years ago

getSymbolsFromNM should run on a worker thread

Categories

(WebExtensions :: Experiments, defect, P1)

defect

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mstange, Assigned: dthayer)

References

Details

(Whiteboard: triaged)

Attachments

(3 files)

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: → 1362788
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.
Ok, let's give it a try then.
should kris or john thayer (sp?) take this?
Flags: needinfo?(amckay)
Priority: -- → P1
Whiteboard: triaged
Is this something run when the gecko profiler is run, or all the time?
Flags: needinfo?(amckay)
The former.
Would you be able to look at this Doug?
Flags: needinfo?(dothayer)
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.
(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 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+
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
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
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.