29338 - PR_Poll should not poll (spin) on Mac

Status: VERIFIED FIXED

(Core :: Networking, defect, P4)

Description

[Warren Harris]

Right now, the mac implementation of PR_Poll sits in a loop with a Sleep call -- 
when the sleep times out it checks for i/o completion. If we make this timeout 
smaller, we get better i/o throughput (see bug 25108), but that may have the 
effect of starving other threads because we spin, checking too often.

What really needs to be done here, is PR_Poll should wake up in response to 
async i/o completion routines. We can leverage the 
PR_Mac_PostAsyncNotify/PR_Mac_WaitForAsyncNotify stuff that Gordon did for async 
dns to do this.

Comment 1

[Warren Harris]

Moving post beta bugs to M18 which is now the post-beta milestone.

Comment 2

[Gagan]

warren: should this be considered for nsbeta3? Do we know what performance 
enhancements we get? pls. reassign to gordon if you think this needs to be done 
by nsbeta3

Comment 3

[Warren Harris]

This never got done? Ugh. Do we have any profiling tools for the mac? I suspect 
that this seriously eats into our performance while downloading.

Comment 4

[Warren Harris]

Simon: This should go on your "mac doesn't suck" list.

Comment 5

[Simon Fraser [no longer active]]

Add the needinfo status. Someone needs to figure out how much of a performance 
hit this is causing. Gordon?

Comment 6

[Simon Fraser [no longer active]]

Reassign to gordon per Mac meeting today.

Comment 7

[Gagan]

getting it on the correct nsbeta3+ radar.

Comment 8

[lchiang]

per PDT: P3-P5 priority bugs changed from nsbeta3+ to nsbeta3- since we have
more important work to do for Seamonkey. If you disagree, please state your case
in the bug report and nominate for rtm and adjust priority if this bug was
mis-prioritized. Thanks.

Comment 9

[Simon Fraser [no longer active]]

Gordon: please report progress in this bug.

Comment 10

[Simon Fraser [no longer active]]

Nominate for 0.9. M18 is past, you know

Comment 11

[Mike Pinkerton (not reading bugmail)]

cleanup

Comment 12

[Doug Turner (:dougt)]

When PR_Poll wakes up there should be some async IO ready to be processed. 
Currently, the mac implemention wakes up when there isn't any.  This caused some
hackery in the nsSocketTransport which we have to check to see if the available
on the transport is zero on the mac and if so bump it to some positive number. 
This seams just really wrong.

What is the status of making PR_Poll on the mac not suck?     

Comment 13

[Mike Pinkerton (not reading bugmail)]

re-cleaning up after dougt blasted away my changes.

Comment 14

[gordon]

PR_Poll on Mac should be rewritten to use PR_WaitForAsyncNotify (read the 
beginning of this description).  I'm working on the cache at the moment.  If 
another Mac engineer familiar with NSPR wants to take a crack at it, great.  
Otherwise, it will have to wait until the first phase of cache development is 
done.  Fixing the cache will probably make a much bigger improvement to Mac 
performance than fixing this bug.

Comment 15

[Simon Fraser [no longer active]]

Gordon and I looked into this some more today. It turns out that to really 
achieve parity with other platforms, and get good performance benefits, we need 
to fix both PR_Poll, and to implement Pollable Events on Mac. This is because the 
socket transport in Mozilla calls PR_Poll with a short timeout (5ms) if pollable 
events are not available. This is conditionalized with #ifdef USE_POLLABLE_EVENT 
in nsSocketTransportService.

Implementing pollable events should be fairly straightforward; on Mac, you would 
have to do little more than wake the PR_Poll thread when the pollable event is 
set.

Another task that should be done at the same time is to unify the two async IO 
thread notification mechanisms we have now (WaitOnThisThread() vs. 
PR_Mac_WaitForAsyncNotify()).

Gordon won't have time to get to this bug until after Necko cache work is done.
Do we have any other takers?

Comment 16

[Steve Dagley]

Looks like I need to talk to wtc about restoring my NSPR checkin privs

Comment 17

[gordon]

Do you have a patch ready?  Should we get together with Simon or Patrick to 
review it?

Comment 18

[Steve Dagley]

No, I was just anticipating having to check something in :-)

Comment 19

[Simon Fraser [no longer active]]

So it turns out that pollable events _do_ work on Mac. They just open a socket to 
localhost, and write into it. So the fix for this bug is rather simple; just 
ensure that we save the thread on which PR_Poll is being called for each 
PollDesc, and, in the notifier routine, wake this thread (rather than the read or 
write thread). I also fixed nsSocketTransportService.h to tell it that we have 
pollable events on Mac.

Comment 20

[Simon Fraser [no longer active]]

Attachment: NSPR changes for PR_Poll fix

Comment 21

[Simon Fraser [no longer active]]

Attachment: Necko changes for PR_Poll fix

Comment 22

[Simon Fraser [no longer active]]

Disclaimer: I really don't know what I'm doing in NSPR. These patches appear to 
work for me, and FTP and mail still work. But YMMV, emptor caveat, results are 
not typical.

Comment 23

[Steve Dagley]

I'll get these patches into a build later today and beat on them (and being on a 
100BaseT switched network in B20 should make for some interesting perf testing :-
)

Comment 24

[Simon Fraser [no longer active]]

Questions for Wan-Teh:
1. Can we assume that PR_Poll is called on the same thread from which reading and
   writing occurs? (The patch does not assume this).
2. Can we assume that waking the PR_Poll thread from our I/O notifier will be
   sufficient to later wake the read/write threads (if different)? The patch 
   currently assumes this.
3. Finally, the changes in prsocket.c are unfortunate, in that this is not a mac-
   only file. Is there a better place to set the polling thread member of the
   polled fd?

Comment 25

[Wan-Teh Chang]

(I am going on sabbatical this Saturday, March 3.)

Answers to Simon's questions.
1. No, we can't assume that PR_Poll is called on the same
   thread from which reading and writing occurs.
2. No, waking the PR_Poll thread from our I/O notifier will
   not be sufficient to later wake the read/write threads
   if different.
3. A better place to set the polling thread member of the
   poll fd would be in _MD_poll() in macsockotpt.c.

Comment 26

[Simon Fraser [no longer active]]

Attachment: Fix in nsSocketTransportService.cpp to account for the pollable event socket

Comment 27

[Doug Turner (:dougt)]

r=dougt on patch 26467

Comment 28

[Simon Fraser [no longer active]]

Attachment: Final NSPR diffs

Comment 29

[Simon Fraser [no longer active]]

Attachment: Final netwerks diffs

Comment 30

[Simon Fraser [no longer active]]

I attach final diffs for checkin. The changes I made are:
1. NSPR:
   i) moved setting the poll thread into _MD_Poll, rather than #ifdef XP_MAC
      code in prsocket.c.
   ii) included the GetState fix which is described in bug 70408

The necko changes include both necko patches already attached to this bug.

Comment 31

[Simon Fraser [no longer active]]

Note: with these changes, I tested page loading, IMAP, HTTP over SSL, activation, 
SMTP, and everything worked fine.

Comment 32

[gordon]

I just checked in Simon's patches to Necko, NSPRPUB_CLIENT_BRANCH, and NSPR tip.

Comment 33

[Simon Fraser [no longer active]]

Taking

Comment 34

[Simon Fraser [no longer active]]

Fixed.

Comment 35

[Tom Everingham]

verified