1516117 - Optimize locking in ChannelEventQueue::CompleteResume

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

This shows heavily as a blocker (~1ms on a high perf hardware) for big number of resources, e.g. on https://www.facebook.com/DwayneJohnson/.

We enter and release the lock 3 times.  This can be easily reduced to 1 enter and leave instead.

Ideally, rewrite the code as lock-less, but I'm afraid that is not easily doable.

Comment 1

[Honza Bambas (:mayhemer)]

self-time here can go up to 5ms in few cases, most of them are 1.5+ms, actually.

Comment 2

[Honza Bambas (:mayhemer)]

Question is if this is the locking.  It more seems like the events being processed from inside cause the delay.

Comment 3

[Stephen Donner [:stephend] Not actively reading bugmail]

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Comment 4

[Valentin Gosu [:valentin] (he/him)]

A perf profile would also be useful here.
P3 for now.

Comment 5

[Kershaw Chang [:kershaw]]

Increased this to P2, since it's an easy change.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

We do lots of locking.

https://searchfox.org/mozilla-central/rev/0529464f0d2981347ef581f7521ace8b7af7f7ac/netwerk/ipc/ChannelEventQueue.h#319,333,343,354
https://searchfox.org/mozilla-central/rev/0529464f0d2981347ef581f7521ace8b7af7f7ac/netwerk/ipc/ChannelEventQueue.cpp#39,46,56,81,108

Comment 7

[Oskar Mansfeld]

Attachment: Bug 1516117 - Optimized locking in ChannelEventQueue::CompleteResume/MaybeFlushQueue/EndForcedQueueing/FlushQueue() r=#necko

Comment 8

[Oskar Mansfeld]

Just pushed a WIP. I think the locks I removed were relatively straight forward, but I'm not sure about FlushQueue(), because there are lots of locks in there and also stuff happening in between that doesn't need locking (see https://searchfox.org/mozilla-central/source/netwerk/ipc/ChannelEventQueue.cpp#39,46,56,81,86,103,108,113,120,125).

So in my mind it didn't make sense to put that whole function under a single lock, because then we are stopping other threads from accessing member vars for a prolonged time. I'm not sure about the performance tradeoffs here (or how to test them). That's why I called mMutex.Unlock() before calling FlushQueue() in MaybeFlushQueue() , but I'm also not sure if that's the correct pattern for unlocking before scope ends.

Any input is appreciated!

Comment 9

[Sunil Mayya]

Hey Oskar,

Just pushed a WIP. I think the locks I removed were relatively straight forward, but I'm not sure about FlushQueue(), because there are lots of locks in there and also stuff happening in between that doesn't need locking (see https://searchfox.org/mozilla-central/source/netwerk/ipc/ChannelEventQueue.cpp#39,46,56,81,86,103,108,113,120,125).
So in my mind it didn't make sense to put that whole function under a single lock, because then we are stopping other threads from accessing member vars for a prolonged time. I'm not sure about the performance tradeoffs here (or how to test them). That's why I called mMutex.Unlock() before calling FlushQueue() in MaybeFlushQueue() , but I'm also not sure if that's the correct pattern for unlocking before scope ends.

The current code (w/o your patch) minimizes critical section duration, reducing the time that other threads are blocked. However, the large number of locks introduces a higher lock acquisition overhead. An optimal solution would be to balance the duration of critical sections and the number of locks used. We need to adjust the code so that critical sections avoid executing any long-running tasks particularly the runnables in the event queue. Hence, I think we can get rid of mMutex.Unlock() as well and further optimize FlushQueue. Apart from this I dont see any other code paths that could block for considerable duration. Kindly share if you come across more such paths.

Please run the code with the thread sanitizers in the try server. Let me know if you need more info on that.

Comment 10

[Oskar Mansfeld]

Thanks for your input Sunil, I just put an updated patch up for review. Link to a build with thread sanitizers is also at Phabricator.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e8cc75512de
Optimized locking in ChannelEventQueue::CompleteResume/MaybeFlushQueue/EndForcedQueueing/FlushQueue() r=necko-reviewers,kershaw,valentin

Comment 12

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/2e8cc75512de