1510262 - nsIncrementalDownload causes a lot of jank with main thread I/O while downloading updates in the background

Status: NEW

(Core :: Networking: HTTP, defect, P2)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Junior [inactive]]

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

Comment 2

[Florian Quèze [:florian]]

(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

[Honza Bambas (:mayhemer)]

Agree, this has to move to a different thread.  Is this is a recent regression?

Comment 4

[Florian Quèze [:florian]]

(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

[Selena Deckelmann :selenamarie :selena]

Honza is this something you can tackle?

Comment 6

[Honza Bambas (:mayhemer)]

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

Comment 7

[:Gijs (he/him)]

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.

Comment 8

[Florian Quèze [:florian]]

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

[Honza Bambas (:mayhemer)]

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.