Closed Bug 29338 Opened 20 years ago Closed 19 years ago

PR_Poll should not poll (spin) on Mac

Categories

(Core :: Networking, defect, P4, major)

PowerPC
Mac System 8.5
defect

Tracking

()

VERIFIED FIXED
mozilla0.9

People

(Reporter: warrensomebody, Assigned: sfraser_bugs)

References

Details

(Keywords: perf)

Attachments

(5 files)

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.
Keywords: perf
OS: Windows NT → Mac System 8.5
Hardware: PC → Macintosh
Target Milestone: M15
Target Milestone: M15 → M17
Moving post beta bugs to M18 which is now the post-beta milestone.
Target Milestone: M17 → M18
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
Assignee: gordon → warren
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.
Simon: This should go on your "mac doesn't suck" list.
Keywords: nsmac2
Add the needinfo status. Someone needs to figure out how much of a performance 
hit this is causing. Gordon?
Whiteboard: [NEEDINFO]
Reassign to gordon per Mac meeting today.
Assignee: warren → gordon
getting it on the correct nsbeta3+ radar.
Keywords: nsbeta3
Priority: P3 → P4
Whiteboard: [NEEDINFO] → [NEEDINFO][nsbeta3+]
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.
Whiteboard: [NEEDINFO][nsbeta3+] → [NEEDINFO][nsbeta3-]
Gordon: please report progress in this bug.
Nominate for 0.9. M18 is past, you know
Keywords: mozilla0.9
cleanup
Severity: normal → major
Keywords: nsbeta3, nsmac2nsbeta1, nsmac1
Whiteboard: [NEEDINFO][nsbeta3-]
Target Milestone: M18 → ---
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?     
Severity: major → normal
Keywords: nsbeta1, nsmac1nsbeta3, nsmac2
Whiteboard: [NEEDINFO][nsbeta3-]
Target Milestone: --- → M18
re-cleaning up after dougt blasted away my changes.
Severity: normal → major
Keywords: nsbeta3, nsmac2nsbeta1, nsmac1
Whiteboard: [NEEDINFO][nsbeta3-]
Target Milestone: M18 → ---
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.
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?
Looks like I need to talk to wtc about restoring my NSPR checkin privs
Assignee: gordon → sdagley
Do you have a patch ready?  Should we get together with Simon or Patrick to 
review it?
No, I was just anticipating having to check something in :-)

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.
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.
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 :-
)
Status: NEW → ASSIGNED
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?
(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.
Target Milestone: --- → mozilla0.9
r=dougt on patch 26467
Blocks: 71668
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.
Assignee: sdagley → gordon
Status: ASSIGNED → NEW
Note: with these changes, I tested page loading, IMAP, HTTP over SSL, activation, 
SMTP, and everything worked fine.
I just checked in Simon's patches to Necko, NSPRPUB_CLIENT_BRANCH, and NSPR tip.
Taking
Assignee: gordon → sfraser
Fixed.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.