Open
Bug 1510262
Opened 4 years ago
Updated 4 months ago
nsIncrementalDownload causes a lot of jank with main thread I/O while downloading updates in the background
Categories
(Core :: Networking: HTTP, defect, P3)
Core
Networking: HTTP
Tracking
()
NEW
Performance Impact | medium |
People
(Reporter: florian, Unassigned, NeedInfo)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: perf, perf:responsiveness, Whiteboard: [necko-triaged][bhr:nsIncrementalDownload])
See this profile captured on the 2018 reference hardware: https://perfht.ml/2RiiwTM This is happening when downloading an update in the background, which can happen while the user is attempting to load pages. Especially as this background update download tends to happen soon after startup.
Updated•4 years ago
|
Whiteboard: [qf] → [qf:p2:responsiveness]
Comment 1•4 years ago
|
||
The jank occurs on closing the fd, and we did the fd open and close every ODA. We might only close the fd on OnStopRequest, but it's risky to modify super old code [1] [1] https://searchfox.org/mozilla-central/rev/0859e6b10fb901875c80de8f8fc33cbb77b2505e/netwerk/base/nsIncrementalDownload.cpp#60
Component: Networking → Networking: HTTP
Priority: -- → P2
Whiteboard: [qf:p2:responsiveness] → [qf:p2:responsiveness][necko-triaged]
Reporter | ||
Comment 2•4 years ago
|
||
(In reply to Junior Hsu from comment #1) > We might only close the fd on OnStopRequest Ideally the whole thing should move off main thread. We only need to send something back to the main thread if we have JavaScript code on the receiving end. http://queze.net/bhr/#filter=nsIncrementalDownload%3A%3AFlush shows we have hangs for the PR_Close that you pointed, but we also have some for PR_Write and for OpenNSPRFileDesc.
![]() |
||
Comment 3•4 years ago
|
||
Agree, this has to move to a different thread. Is this is a recent regression?
Reporter | ||
Comment 4•4 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #3) > Is this is a recent regression? I don't think so, I already noticed this behavior when profiling for Quantum Flow / Firefox 57. It's more likely to be the last occurence that remains of something that was kinda OK a decade ago.
Comment 5•4 years ago
|
||
Honza is this something you can tackle?
Assignee: nobody → honzab.moz
Flags: needinfo?(honzab.moz)
![]() |
||
Comment 6•4 years ago
|
||
Following calls hit PR_Close(): OnStartRequest: https://searchfox.org/mozilla-central/rev/3160ddc1f0ab55d230c595366662c62950e5c785/netwerk/base/nsIncrementalDownload.cpp#616 OnDataAvailable: https://searchfox.org/mozilla-central/rev/3160ddc1f0ab55d230c595366662c62950e5c785/netwerk/base/nsIncrementalDownload.cpp#690 OnStopRequest: https://searchfox.org/mozilla-central/rev/3160ddc1f0ab55d230c595366662c62950e5c785/netwerk/base/nsIncrementalDownload.cpp#653 Solution: - retarget the channel to stream transport service - have a task queue, as at [1] - all writes to the file will happen there - all notifications (start/progress) must go the main thread and after file operations (truncate in onstart, final write on onstop) are done; this is a precaution, not something I checked on we really must do [1] https://searchfox.org/mozilla-central/rev/3160ddc1f0ab55d230c595366662c62950e5c785/netwerk/base/nsNetUtil.cpp#1433-1447
Status: NEW → ASSIGNED
Flags: needinfo?(honzab.moz)
Comment 7•4 years ago
|
||
https://perfht.ml/2GFHcEG has us janking for a combined total of about 15 seconds (7k samples on Windows for nsIncrementalDownload::FlushChunk, plus some more elsewhere) because it's downloading a complete mar file, on the reference hardware with its disk + cpu contention... fixing this bug would really help quite a lot for those cases.
Reporter | ||
Comment 8•4 years ago
|
||
I find the jank in this startup profile even more impressive: https://perfht.ml/2Ezid1L Probably because I was using the fast internet connexion on a machine with slow I/O, so the main thread I/O blocked the whole time.
![]() |
||
Comment 9•4 years ago
|
||
This could be nicely fixed with bug 1528285. Looking more into the code, observer is only an nsIRequestObserver which we can easily dispatch to the main thread. Then there is mProgressSink, for which we can do the same.
Adding dep, should it significantly delay, I can move only the writes/flushes to a bck thread.
Depends on: omt-networking
![]() |
||
Updated•3 years ago
|
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Updated•3 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Updated•2 years ago
|
Whiteboard: [qf:p2:responsiveness][necko-triaged] → [qf:p2:responsiveness][necko-triaged][bhr:nsIncrementalDownload]
Updated•1 year ago
|
Blocks: necko-perf
Updated•1 year ago
|
Priority: P2 → P3
Updated•11 months ago
|
Performance Impact: --- → P2
Keywords: perf:responsiveness
Whiteboard: [qf:p2:responsiveness][necko-triaged][bhr:nsIncrementalDownload] → [necko-triaged][bhr:nsIncrementalDownload]
Updated•4 months ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•