71718 - Carbonized Mac NSPR needs architectural work to run on dual-CPU machines

Status: VERIFIED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Mike Pinkerton (not reading bugmail)]

Run the fizzilla build on a dual-cpu g4 (like the one in my cube) and the app 
stalls loading most pages. Here is the email from Apple ([e-mail]):

Mike,

I looked at the code on macsockotpt.c and do not see any call to
OTEnterNotifier() and OTLeaveNotifier().

On Mac OS X, Open Transport notifiers, timer tasks and deferred task do not
run to completion. That means that code from the main event loop or another
thread may preempt code running from OT notifiers, OT timer tasks and OT
deferred tasks. The solution is to always protect your data by using either
atomic operations or by using OTEnterNotifier()/OTLeaveNotifier().

Bracketing your critical sections with OTEnterNotifier()/OTLeaveNotifier()
is usually the most convenient solution as this ensures that either the
notifier or the main event loop / cooperative thread is accessing common
data.

For example here is what I would do:

1369             do {
                     Boolean entered = OTEnterNotifier(endpoint);
1370                 fd->secret->md.read.thread = me;
1371                 fd->secret->md.readReady = PR_FALSE;                //
expect the worst   
1372                 result = OTRcv(endpoint, buf, bytesLeft, NULL);
1373                 if (fd->secret->nonblocking) {
1374                     fd->secret->md.readReady = (result !=
kOTNoDataErr);
                         if (entered)
                             OTLeaveNotifier(endpoint);
1375                     break;
1376                 } else {
1377                     if (result != kOTNoDataErr) {
1378                         // If we successfully read a blocking socket,
check for more data.
1379                         // According to IM:OT, we should be able to
rely on OTCountDataBytes
1380                         // to tell us whether there is a nonzero amount
of data pending.
1381                         size_t count;
1382                         OSErr tmpResult;
1383                         tmpResult = OTCountDataBytes(endpoint, &count);
1384                         fd->secret->md.readReady = ((tmpResult ==
kOTNoError) && (count > 0));
1385         
                             if (entered)
                                 OTLeaveNotifier(endpoint);
1386                         break;
1387                     }
1388 
1389                     // Blocking read, but no data available. Wait for
an event
1390                     // on this endpoint, and hope that we get a T_DATA
event.
                         if (entered)
                                 OTLeaveNotifier(endpoint);
1391                     WaitOnThisThread(me, PR_INTERVAL_NO_TIMEOUT);
1392                     result = me->md.osErrCode;
1393                     if (result != kOTNoError) // interrupted thread,
etc.
1394                         break;
1395 
1396                     // Prepare to loop back and try again
1397                     PrepareForAsyncCompletion(me, fd->secret->md.osfd);
1398                 }
1399             }
1400             // Retry read if we had to wait for data to show up.
1401             while(1);

It certainly behaves like our endpoints are being clobbered in a race-condition-
ish situation.

Comment 1

[Steve Dagley]

Apple has shipped us a dual CPU G4 so we don't have to camp in pink's cube trying 
to debug this

Comment 2

[Simon Fraser [no longer active]]

They have? Can I have it?  ;)

Comment 3

[Steve Dagley]

sfraser has the dual CPU loaner Mac, now he has the bug too :-)

Comment 4

[rubydoo123]

moving to 9.2

Comment 5

[Steve Dagley]

I'll just point out that the dual-CPU loaner system is due back to Apple on 6/1 
so that, while this fix may not be critical for 0.9.1, it needs to land well 
before 0.9.2

Comment 6

[benc]

mass move, v2.
qa to me.

Comment 7

[Simon Fraser [no longer active]]

The Mac NSPR code has serious issues on Mac OS X, which are particularly visible
on machines with two processors.

The main issue here is that OT notifiers, async file I/O callbacks, and timer
tasks each use pthreads under the hood, and are thus interruptible. This breaks
assumptions that the mac code currently makes relating to the switching on and
off of interrupts, and lock consistency.

It would appear that the only good solution to this problem is to build NSPR as
a Mach-O binary, and have it use the Unix implementation. If we do that, we need
to ensure that the implementation permits clients to call any Carbon Toolbox
functions on threads, as they can now. It would be bad to move to pthreads, and
to find, suddenly, that bad things happen when trying to do, say, drawing, or
memory allocation, on non-main threads.

Comment 8

[Wan-Teh Chang]

Steve, could you please work on this bug?  We already
have an implementation of NSPR for Mac OS X based on
pthreads.

Comment 9

[Joe Francis]

wtc, can you tell me which nspr defines need to be lit to do a nspr for mac os x 
build?

Comment 10

[Wan-Teh Chang]

The only way to build NSPR for Mac OS X is to use the Unix build
instructions, i.e., configure and make.

I don't know how to integrate this with Code Warrior.

Comment 11

[Steve Dagley]

I spoke to Quinn at Apple's WWDC about calling into Mach-O from CFM and he 
pointed me to <http://developer.apple.com/samplecode/Sample_Code/
Runtime_Architecture/CallMachOFramework.htm>.  It's intended for calling 
frameworks but I'm hoping it can be adapted to general Mach-O libs.

Comment 12

[Joe Francis]

Thanks.  As it happens I found that on my own.  And I got that much working... I 
can load the System framework at _MD_EARLY_INIT or whatever it's called.  But 
then I'm stuck.  I have no pthread headers that will work with CW.  In fact, just 
to get the first part working I had to copy over the Universal folder from CW7EA.  
There was needed stuff missing from the headers otherwise.  

I tried to handroll my own pthread declararions, because I need only a few calls.  
But unfortunately initializing a pthread mutex requires a *structure* as one of 
the params (what the heck were those posix folks thinking?) so it gets 
complicated to get it all correct.  I really need posix headers that work with 
CodeWarrior.

Or I could try porting nspr over to ProjectBuilder.  Then I could make a knock-
off nspr lib that had pthreads locks in the right places to get dual cpu macs 
happy.

Comment 13

[Joe Francis]

oops.  Sorry Steve, I thought you were replying to a question I asked in another 
forum.  You are actually looking at the opposite solution from what I am 
thinking.  You want to make a mach-o nspr and clal it from cfm fizzilla, right?  
I want to make the current cfm nspr use pthread locks from the system framework 
make the INTS_OFF/INTS_ON stuff in nspr actually acquire a real lock that the 
underlying os will honor.

I'm worried about using real pthreads in mac nspr because I think you can only 
make most system calls from the main thread and I don't know that we can enforce 
that restriction on the current code base.  So I'm thinking keep the nspr threads 
and just enforce syncronization with the callbacks/timers/notifiers using a real 
lock.

Comment 14

[Joe Francis]

ok, new idea.  make the INTS_ON/INTS_OFF stuff enter and leave a critical
region.  That way everything turning on/off nspr ints is competing on the same
critical region, acting just like a lock.  The reason this is good is that the
mac MP api supports this, and on mac os x it is just a wrapper for posix stuff,
so the underlying os threads running the callbacks/timers/notifiers will respect
this.  I'll wager the MP headers don't pose any problems for our build system,
so it should be easy to do.  i'll try it later today and see.

Comment 15

[Mike Pinkerton (not reading bugmail)]

two thoughts:
- pro7 can build mach-o, so why aren't all the pthread headers there?
- aren't the correct pthread headers on the OSX developer install for you to
look at?

Comment 16

[Simon Fraser [no longer active]]

Yes, you will be able to call MP stuff. This might be one way to go.

Comment 17

[Mike Pinkerton (not reading bugmail)]

beta stopper

Comment 18

[Joe Francis]

I've played around with this some.  My first attempt seems to just lock all the
threads up.  The chief difficulty is that _PR_INTSOFF retreives a value that
indicates whether the region is already entered.  But the MP api does not expose
any way to get that info for a critical region.  The result is that the nspr
code triggers more entries into the critical region than exits.

I think it may be possible to make _PR_INTSOFF & _PR_INTSON not care about that
value.  The number of places in the code that actually examine that value are
few, and are almost entirely in mac only code.  I think this value is only
needed because _PR_INTSOFF doesn't currently acquire a "real" lock of any kind. 

I'll play with this some more over the weekend.

Comment 19

[gordon]

How about keeping a counter for the number of times _PR_INTSOFF has been called, 
and only enter/leave the critical section on 0/1 transitions, sort of like 
destructors and reference counting.  That should have similar semantics to the 
current usage of _PR_INTSOFF.

Or it may be even simpler than that.  You may just need to keep a flag indicating 
whether you're in the critical section, and return that flag.

If you're around this afternoon, I'd love to talk with you about it.

Comment 20

[Mike Pinkerton (not reading bugmail)]

you would probably need a separate lock on this counter too, as you could be
pre-empted between checking the value and then acting on it causing a
race-condition.

Comment 21

[Joe Francis]

re gordon: i was already trying that.
re mike: i was entering my critical section before using the counter.

there is some code around that saves the counter, and then enters the intsOff
state setting the counter to 1, and then restores the counter when it's done. 
This will have the effect of not turning ints back on if they weren't on before,
but it totally demolishes the concept of the counter being a reentrancy count.  
i think i can just not use a counter at all, and tweak the code in the handful
of places that use it.  Almost all are in mac only code, and most of it is to
detect when a notification might have been dropped becasue it came while ints
were off.  Under osx the notification wont be dropperd in that situation: if
someone else is in the critical section when the notification comes in, the
notification will block on the critical section, and then fire when it can enter.  

Comment 22

[Joe Francis]

at request of macdev, attaching my patches.  they  DO NOT work.  But they 
show the lines I am thinking along.  

Comment 23

[Joe Francis]

Attachment: diffs of nsprpb

Comment 24

[Joe Francis]

a second attempt.  not sure yet why this one doesn't work.

Comment 25

[Joe Francis]

Attachment: nspr patch

Comment 26

[lchiang]

sdagley - are any of the machines which we'll get from Apple for testing a
dual-CPU machine?  If dual-CPU machine seems to have it's own set of problems,
we may need to test on that.

Comment 27

[Simon Fraser [no longer active]]

We should *definately* test on dual-CPU machines, and it would be good for QA to 
invest in a few of these. Note that, currently, our builds are pretty much dead 
on such machines running Mac OS X because of this bug.

Comment 28

[Steve Dagley]

lchiang, we aren't getting any dual CPUs in the new batch but Joe Francis has 
voluntereed his dual-cpu system for testing while he's on sabbatical next month

Comment 29

[Marek Z. Jeziorek]

Steve, this is PDT+ bug. Tomorrow, on Tuesday, we'll try to build the first RTM
candidate. It would be good, if this could be resolved ASAP.

Comment 30

[chris hofmann]

what going on with this bug?  getting close to the time where it may have to be
latered to future milestone/release if we are not there, or past there already...

Comment 31

[Steve Dagley]

This bug is specific to Mac OS X and does not impact the 'Classic' builds for Mac 
OS 8.6/9.x.  As to where we're at with it, as an AOL employee I can't use the 
appropriate profanity in this public forum to describe it so I'll just say we 
don't expect to have a fix this month.

Comment 32

[Boris Zbarsky [:bzbarsky]]

*** Bug 90941 has been marked as a duplicate of this bug. ***

Comment 33

[Mike Pinkerton (not reading bugmail)]

we're not gonna have this fixed by beta, removing pdt+

Comment 34

[Mike Pinkerton (not reading bugmail)]

*** Bug 93824 has been marked as a duplicate of this bug. ***

Comment 35

[Mike Pinkerton (not reading bugmail)]

*** Bug 93720 has been marked as a duplicate of this bug. ***

Comment 36

[Adam Masri]

The following Apple tech note may be helpful in resolving this bug, as well as
giving Fizzilla better OS X performance.

http://developer.apple.com/technotes/tn/tn2028.html

- Adam

Comment 37

[Tiantian Kong]

Added keyword: nsenterprise+

Comment 38

[Steve Dagley]

Simon is now working on this so reassigning bug

Comment 39

[Simon Fraser [no longer active]]

Attachment: work in progress patch. needs some cleanup.

Comment 40

[Simon Fraser [no longer active]]

Attachment: Final patch for review

Comment 41

[Simon Fraser [no longer active]]

That last patch works. On single CPU machines, it's performance impact is below
the noise threshold of the page loading tests I used to measure it. It uses MP
critical sections to fix thread synchronization issues, blocking multiple MP
threads from running while NSPR interrupts are turned off.

The code is currently active if TARGET_CARBON is defined to be 1. It might be
necessary to detect the OS version using Gestalt if this affects Carbon under 9
performance (which is unknown).

I need some strong reviews, please.

Comment 42

[Simon Fraser [no longer active]]

I have some further changes to the patch, to conditionalize the critical region 
code for Mac OS X only, and to add some explanatory comments. Please wait...

Comment 43

[chris hofmann]

is this going to make it soon?  putting on the "+"

Comment 44

[Simon Fraser [no longer active]]

Attachment: Really final patch

Comment 45

[Simon Fraser [no longer active]]

The latest patch does two new things:
1. Return non-Carbon builds to their previous behaviour (just some macro changes).
2. In carbon builds, conditionally uses critical sections only if we are running
on Mac OS X. So Carbon-on-classic builds are not penalized.

Comment 46

[Simon Fraser [no longer active]]

Attachment: New patch addressing wtc's email comments

Comment 47

[Simon Fraser [no longer active]]

The new patch:
* Replaces the array of critical sections with just one (since I only used one),
  which now matches the single critical region entry count.
* Removes a bogus WalkTheStack prototype.

Comment 48

[Wan-Teh Chang]

I reviewed Simon's "really final patch" (attachment 49391 [details] [diff] [review]).
I already sent him some suggested changes after a quick
look.  Then I stared at the patch for a good hour.  After
I internalized the patch, I found that Simon's solution is
actually very simple.  (It is all those macros that makes
the patch look complicated.) It is exactly what jfrancis
outlined in his comments back in June.  It surprises me that
the simple solution is enough to solve this problem.
Congratulations to all of you who worked on this bug!

I will go over the patch with Simon next week.  I just
wanted to point out that the patch also contains a fix
for bug 97866.  That bug is marked as depending on this
bug, but I think it is not related and its fix should
be reviewed and checked in independently.

Comment 49

[lchiang]

(This looks close to a fix - great!  Who at Netscape has a dual-processor
machine besides kin?  Once this is fixed, we need to do some testing.)

Comment 50

[Jaime Rodriguez, Jr.]

PDT+ per PDT. Let's get one.

Comment 51

[benc]

Sorry for the somewhat basic quizzing but:

1- This bug discusses fixing the MacOS X builds only right? So it is completely
irrelevant to running MacOS X w/ classic and classic build.

2- What happened if you ran MacOS X builds on dual-processor systems before this
fix? (Boom?!). Did this affect dual processor systems in pre-X?

Need this info to figure out how much upgrading is needed for testing.

Comment 52

[Mike Pinkerton (not reading bugmail)]

ben:

1) correct, this does not affect the classic bits (non-carbon) running on osx in 
the classic environment

2) pages would stall, and never finish loading. sometimes the app would crash 
when you quit after failing to load pages.

Comment 53

[Simon Fraser [no longer active]]

Attachment: Super-final patch

Comment 54

[Simon Fraser [no longer active]]

The super-final patch:
* removes the async file I/O changes, which are now in a patch in bug 97866
* replaces separate intsoff/intson implemenations with a single, simplified
intsoff routine, and removes some macro code duplication.
* Uses intsoff/fastintson rather than setintsoff(1)/setintsoff(0) in a couple of
callback routines.
* Improves the comments in _MD_PauseCPU and DoneWaitingOnThisThread().

gordon, please review.
wtc, if you'll then bless this, I can check it in.

Comment 55

[gordon]

r=gordon, unless you want to clean up _macos.h or add a wrapper to
DoneWaitingOnThisThread().

wtc, the #ifdefs in primpl.h give me headaches.  Are you sure they're correct? 
There seem to be redundant nesting of global and local thread cases.

Comment 56

[Simon Fraser [no longer active]]

And now the bad news. Adding the critical regions on Mac OS X does have some 
perceptible impact on performance in some areas. Page loading is not affected, 
but some UI operations are (I noticed, for example, that loading the buddy list 
in a commercial build got slower), and the browser feels someone less responsive 
on X. It may be that parts of the code that rely a lot on PR_Lock/PR_Unlock, or 
atomic operations, are slowed down by this patch.

Comment 57

[Jaime Rodriguez, Jr.]

This is not on Tian Tian Must Fix list for this release. Should we consider
minusing it, and changing the priority for this release?

Comment 58

[Brian Nesse (gone)]

sdagley commenting here - If this isn't on tiantian's list it damn sure should
be - this is most DEFINITELY a stop ship bug.  Otherwise explain to Marty Fisher
hy we don't run on his new Mac :-)

Comment 59

[Joe Francis]

Seconding Steve's comments.  As someone involved with the evolution of this bug 
fix I feel we are much better off with it than without it.  Both MacOS X and dual 
CPU macs are catching on pretty quickly.  Lets not leave those folks stranded.

Comment 60

[Tiantian Kong]

Hi:
 This bug is the FIRST one listed on my stop ship bug list compiled yesterday.
Please check again the list on
http://bugscape.netscape.com/show_bug.cgi?id=9485. And it is the first bug on
the 8/21 list as well. 

Comment 61

[Jaime Rodriguez, Jr.]

Check it into the trunk, and let's get some testing numbers (New Window,
Startup, etc.). Tian Tian and Hong to coordinate test efforts.

Comment 62

[Simon Fraser [no longer active]]

Attachment: Patch with minor revisions

Comment 63

[Simon Fraser [no longer active]]

Chnages in this patch: 
* removed redundant duplicate of _MD_INTSOFF macro in _macos.h
* In code blocks like this:

    if (_PR_MD_GET_INTSOFF()) {
        thread->md.missedIONotify = PR_TRUE;
        cpu->u.missed[cpu->where] |= _PR_MISSED_IO;
        return;
    }

which are outside of the critical region, reversed the order of setting
missedIONotify and the _PR_MISSED_IO to mitigate the effects of preemption at
this point.

Note that the patch is a diff -b (ignoring space change), so spacing looks worse
than it really is.

Comment 64

[gordon]

Comment on attachment 49998 [details] [diff] [review]
Patch with minor revisions

r=gordon

Comment 65

[gordon]

I was unable to edit patch status to specify "has-review". Does that have
something to do with the Product field?

Comment 66

[lchiang]

I will coordinate the testing for the perf numbers.  Results to be posted when 
they come in.  Per simon:

1.  Tests run on single processor CPU (we don't have dual handy)
2.  Run on today's trunk build vs. tomorrow's trunk build
3.  Run:  window open, startup, IM, prefs, mail performance tests
4.  Results posted back here as attachments

Thanks

Comment 67

[Wan-Teh Chang]

Attachment: Proposed patch, based on sfraser's last patch.

Comment 68

[Wan-Teh Chang]

Most of the changes in my patch from Simon's last patch
are a matter of style.
- I make sure no lines are longer than 80 characters.
- I do not make whitespace changes.  This is to make the
  diffs cleaner.
- Whether tabs or spaces are used follows the local
  convention.  Again, this is to make the diffs easier
  to read.
- "Regions" and "REGIONS" are changed to "Region" and
  "REGION" because there is only one critical region.
- "TermCriticalRegions" is renamed "DestroyCriticalRegion".
- Some macro definitions were moved between primpl.h and
  _macos.h and the intermediate macro _MD_INTSOFF() was
  deleted.
- gUseCriticalRegion and gCriticalRegion are declared static
  because they are only used in macthr.c.

The real changes are:
- In _PR_INTSOFF(), I leave the critical region immediately
  so that the entry count is always 1.  This way, in
  _PR_FAST_INTSON() and _PR_INTSON(), I only need to leave
  the critical region once if we need to leave the critical
  region completely.  As a result, the global variable
  gCriticalRegionEntryCount can be deleted.
- In macio.c, AsyncIOCompletion(), _PR_INTSOFF() and
  _PR_FAST_INTSON() are not necessary because they are called
  in DoneWaitingOnThisThread().  I also moved the setting of
  thread->md.osErrCode before the if block.

Simon, please review my patch and test it.  If it doesn't
work and it takes too long to fix it, you can go ahead and
check in your patch on the NSPRPUB_PRE_4_2_CLIENT_BRANCH.
I will check email from home tonight.

Comment 69

[Wan-Teh Chang]

The sentence
  In _PR_INTSOFF(), I leave the critical region immediately
  so that the entry count is always 1.
should read
  In _PR_INTSOFF(), I leave the critical region immediately
  if we are already in the critical region so that the entry
  count is always 1.

Comment 70

[Simon Fraser [no longer active]]

wtc, your final patch does not work, alas. As-is, it deadlocks on launch. I 
noticed that this seems wrong:

 #define _PR_INTSOFF(_is) \
     PR_BEGIN_MACRO \
+        ENTER_CRITICAL_REGION(); \
         (_is) = _PR_MD_GET_INTSOFF(); \
         _PR_MD_SET_INTSOFF(1); \
+        if (_is) \
+            LEAVE_CRITICAL_REGION(); \
     PR_END_MACRO
 

and should probably be

         _PR_MD_SET_INTSOFF(1); \
+        if (_is == 0) \
+            LEAVE_CRITICAL_REGION(); \

but with that fixed, we get a slew of assertions about leaving non-existent 
critical sections.

Comment 71

[Simon Fraser [no longer active]]

I checked my previous patch into the NSPRPUB_PRE_4_2_CLIENT_BRANCH at 8:45pm, so 
it will show up in trunk builds tomorrow (9/20).

Comment 72

[sairuh (rarely reading bugmail)]

trunk testing [BEFORE checkin] using:

* 2001.09.18.20-trunk commercial
* on Mac OS 10.0.4, 450 Mhz G3, 384Mhz
* modern theme [note: separate profiles used for prefs and page load tests]

Preferences dialog open: 2.50sec
Switching from Navigator to Mail & Newsgroups panel: 0.70sec

will soon attach page load results [side note: crashed several times after page
load completed --see bug 100852].

Comment 73

[sairuh (rarely reading bugmail)]

Attachment: 2001.09.18.20-trunk comm page load results

Comment 74

[sairuh (rarely reading bugmail)]

Comment on attachment 50169 [details]
2001.09.18.20-trunk comm page load results

re-ran the pre-checkin page  load tests, thus invalidating this run. will reattach new set.

Comment 75

[sairuh (rarely reading bugmail)]

Attachment: two runs of page load results using 2001.09.18.20-trunk

Comment 76

[sairuh (rarely reading bugmail)]

tried running the page load tests using the 2001.09.20.16-trunk bits: alas, it
would not complete. it hung [but not on the same page] each of the three times i
tried --even after rebooting the machine after the second hang. :(

will try to run the preferences perf tests. hopefully that will go more smoothly...

Comment 77

[sairuh (rarely reading bugmail)]

alas, i'm blocked with the preferences testing as well. :(

i need to restart the app btwn each dialog-open timing [to get four timings and
report their average]. i was able to launch the app a first time with a new
profile...but with the second launch the browser window didn't even appear: only
got the menubar and the rainbow spinning cursor.

restarting the machine kinda "works around" the hang, but it's a rather harsh
workaround.

another note: simon mentioned that this might be a 10.0.4 issue --since he was
recently able to run the page load tests [same 2001.09.20.16-trunk build] on his
dual-cpu G4 [which has a later version of 10.x].

Comment 78

[sairuh (rarely reading bugmail)]

recent email rec'd from [e-mail]:

"...I've been following this bug since I want to use mozilla on my dual 800 with
OS X 10.0.4.

"I just noticed your most recent comment at 22:17, and wanted to let you know
that I am experiencing the same spinning rainbow cursor, and the browser never
starts when using the 2001.09.20 build.

"Hope that helps in any way, let me know if I can help otherwise."

Comment 79

[Simon Fraser [no longer active]]

Hold off on testing here folks. It looks like my changes causes deadlock on 
single and dual-CPU machines running 10.0.x builds. Damn. Running a new build 
now, more tomorrow.

Comment 80

[Jussi-Pekka Mantere]

Running 10.1 would not affect the deadlock?

Comment 81

[Jaime Rodriguez, Jr.]

Let's find out if this will ever work on 10.04? Simon is researching ...

Comment 82

[Brian Bergstrand]

I think 10.0.x is a lost cause. I just got a new dual-800, and was having 
MAJOR stability problems with 10.0.4 4S10. The machine would randomly 
crash. I posted to the Darwin list, and was told 10.0.4 has major SMP 
issues and to wait for 10.1. Let's just say this was good advice.

See http://www.darwinfo.org/devlist.php3?number=11117 for the full 
message.

Comment 83

[[e-mail] (gone - use [e-mail] instead)]

Attachment: OS X perf results for Mail

Comment 84

[lchiang]

Looking at Stephen's mail results for OS X for 9-18 vs. 9-21 builds, there is
some amount of slow down in the 9-21 build.

Comment 85

[Steve Dagley]

Can we get those tests re-run with  Mac OS X 10.abouttobereleased?

Comment 86

[Brice Ruth]

Do you need a dual CPU or does a single CPU work for the speed tests?  If a
single CPU works, lemme know exactly which files to grab for the tests to be
consistent.

Comment 87

[lchiang]

Brice - thanks for the offer to help.  For the speed tests, here is one of them
(Mail).  All the test files and methodology are there. The important thing is to
use the same machine for the before/after builds with simon's fix.   A single
CPU is fine for this speed test.   Do you have version 10.1 of the OS?   If so,
that would be great.

sdagley - can you give me a CD with the updated OS?

Comment 88

[Brice Ruth]

I looked at the perf results attachment and I think I can do all that ... gotta
see about the stopwatch, though :)

If I have any questions, I'll let ya know.

I assume grabbing today's 'nightly' is fine enough for a build w/ the patch
incorporated?

Comment 89

[Simon Fraser [no longer active]]

Here's the deal. The MP APIs that I'm using for dual CPU protection are broken 
under 10.0.x, but fixed for 10.1. I have code that works around this problem, 
that will make things work on 10.0.x. So I need to check that in, and we then 
have to do another round of performance testing.

Comment 90

[Brice Ruth]

Oh - and I can't confirm anything about having 10.1 - to my knowledge, there is
no such release yet. :)

Comment 91

[Simon Fraser [no longer active]]

Attachment: Patch to fix MPEnter/LeaveCriticalRegion on 10.0.x

Comment 92

[[e-mail] (gone - use [e-mail] instead)]

Brice, yes.  Thanks for helping with this.  So, to summarize:

Folder loading speed: done with local folders
Thread pane scrolling: local or IMAP
Message display speed: local folders
Thread pane sorting: local or imap
Message Reply Speed: IMAP, see
http://www.mozilla.org/mailnews/performance/msgcompose_loggin.html for details
on how to generate a performance logfile for this.
Batch delete 5 (10kb each) messages: local
Mail module loading time: I wouldn't bother with this, it's too complex
currently (using IMAP, non-isolated servers, etc.) to be truly reliable.

Comment 93

[grega]

marking nsenterprise- to get off the enterprise stop ship list.

Comment 94

[Simon Fraser [no longer active]]

I tested the lastest changes on 10.1 single- and dual- CPU machines, and both 
work just fine.

Comment 95

[lchiang]

I have the 10.1 update to Mac OS X.  Stephen or Sairuh, pls stop by to install
for testing.

Simon - do you expect to see a timing difference between OS X and 10.1?

Comment 96

[Simon Fraser [no longer active]]

Attachment: Tidied up MDEnter/Leave critical section code

Comment 97

[sairuh (rarely reading bugmail)]

spoke with lisa briefly: stephend upgraded to 10.1, but i'll stick with 10.0.4
for  the nonce --so that we can test with smfr's latest patch [after
review/check-in] on both OS versions.

Comment 98

[Steve Dagley]

Comment on attachment 50724 [details] [diff] [review]
Tidied up MDEnter/Leave critical section code

r=sdagley

Comment 99

[Simon Fraser [no longer active]]

I checked in the working critical region code to the NSPR branch, which is used 
by the mozilla TRUNK. So this bug is now good to go on for testing.

Comment 100

[Joe Francis]

holy crap.  So that's why I couldn't get this to work before - I was doing the 
same thing under 10.0.4

Comment 101

[Jaime Rodriguez, Jr.]

Simon - Pls get the SR=, so we can plus this one, and check it in today.

Comment 102

[Simon Fraser [no longer active]]

This has all the reviews it needs to be checked in to the branch. It's already on 
the trunk.

Comment 103

[sairuh (rarely reading bugmail)]

trunk testing of Preferences panel: compared with my 2001-09-20 16:31 results.
same configuration, with a new profile [modern, no sidebar].

AFTER checkin, using 2001.09.25.20-trunk
----------------------------------------
Open Preferences dialog open: 3.39sec
Switching from Navigator to Mail & Newsgroups panel: 0.97sec


BEFORE checkin, using 2001.09.18.20-trunk
-----------------------------------------
Preferences dialog open: 2.50sec
Switching from Navigator to Mail & Newsgroups panel: 0.70sec

looks like this checkin might've slowed thing down a bit.

currently running page load tests. will attach when finished.

Comment 104

[[e-mail] (gone - use [e-mail] instead)]

I thought we were supposed to test on the 26th builds:

AFTER build:

Trunk build from 9/26 (when it shows up).

If the fix was committed on 9/25 at 7:38pm, how does it show up in the 25-20
builds, which are produced at 8 pm (and presumably take more than ~20 minutes to
pull and build).

Just checking to make sure I test the right build.

Comment 105

[Wan-Teh Chang]

I just wanted to point out the the focus of the QA
testing should be on the stability of this patch.
That is, whether the Mozilla client crashes or
deadlocks with the patch applied.

The current code is broken on Mac OS X.  No matter
how much faster it is, it is broken.

If Simon's patch fixes the thread safety problem
on Mac OS X, it should be checked in.  There is not
much point in comparing its performance with the
performance of broken code on Mac OS X.  Any
performance comparision should be done on classic
Mac OS, where the current code works.

Comment 106

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 107

[sairuh (rarely reading bugmail)]

i've created bug 101838 for the performance slowdown i've noticed in the
2001.09.25.20-trunk builds.

Comment 108

[[e-mail] (gone - use [e-mail] instead)]

Attachment: Mail performance results, take 2 (10.1, this time)

Comment 109

[Jennifer Kobayashi]

Startup Results
I take an average of three for launch and relaunch on both builds. I tested 
these manually via a stopwatch. Times are in seconds. 
Build 2001-09-18-20-trunk
Launch: 19.25 
Re-launch: 12.49
Build 2001-09-25-20-trunk
Launch: 24.17
Re-launch: 16.62

So, as you can see.. a fairly large jump in times in the 9-25 build :( 
There were no peaks or weird jumps, all times on both builds were fairly 
consistent. 

Comment 110

[Brice Ruth]

OS X v10.1, iMac DV 400MHz, 768MB RAM

Test/Build|--9/18--|--9/27--|
Startup 1 |  24.45 |  18.40 |
Startup 2 |  10.31 |  11.78 |

Startup 1 indicates that I rebooted and launched Mozilla for the first time,
Startup 2 indicates that I launched Mozilla directly after quitting the same
build of Mozilla, after a clean reboot.

Basically, I rebooted, launched build 9/18, quit, launched again - recorded
times, rebooted, launched build 9/27, quit, launched again - recorded times. 
I'll try to get some Mail Perfs now that I have a stopwatch.

Comment 111

[scalkins]

Attachment: IM results of Performance testing on OSX. Everything is slower here also.

Comment 112

[Brice Ruth]

I'm really sorry that I have to report this ... but, I believe I just
experienced deadlock in Mozilla, build 2001092620 - I had the spinning pizza for
at least 5 minutes and when I checked out top, Moz was hovering right around 50%
usage.  I had to forcibly kill Moz :(

I don't really know what 'caused' it ... I was viewing a web page when my wife
sent me an email, so I command-2'd over to Mail/News, hit command-t to get new
mail, then clicked in the 'Denise' folder that her messages get filtered into
and that's when the pizza started spinning.

I don't have a dual-proc machine, this is the same machine I posted the perf
results from (iMac DV 400 w/ 768MB).  If this *wasn't* deadlock, what could it
have been?

Comment 113

[Simon Fraser [no longer active]]

Checked in on 0.9.4 branch. Keeping open for NSPR tip.

Comment 114

[Simon Fraser [no longer active]]

Checked in on the NSPR tip.

Comment 115

[lchiang]

kin - I heard you have a dual processor machine.  Can you verify for QA that
that the branch build starts on your machine?

Comment 116

[sharky_k_]

I have a G4 533 dual and Mozilla build nr. 2001093021 works great. Startup time
could be less (I hope).

Comment 117

[kinmoz]

lchiang - I have no dual processor machines ... I believe both jfrancis and
sfraser do.

Comment 118

[lchiang]

Thank you.

I think I will mark this verified (for startup of application on dual processor
Mac) per [e-mail]'s comment.

I have also asked Marty F. at AOL to confirm for me on his dual processor for
the commercial build.  I will also inquire with one other source for the
commercial build.

Comment 119

[Hong Kwon]

I will also test on fkeeney's dual processor which he has graciously lent to me.
 If anyone wants to come by and use it, you are more than welcome to (B20-1)

Comment 120

[Hong Kwon]

Some results:

On sdagley's G4/733 it took around 5 minutes to download 2220 headers (using
yesterday's trunk)
On fkeeney's dualG4/500 it took around 5 minutes to download 3482 headers (using
today's branch)

On sdagley's G4/733 it took around 30 seconds to download 2220 headers (using a
debug build from a few weeks ago, pre Simon's fix).  This on a non-optimized build.

As you can see this is a HUGE performance degradation.  It is literally 10 times
slower.

Comment 121

[Hong Kwon]

posted new bug for mail performance degradation: 
http://bugzilla.mozilla.org/show_bug.cgi?id=102797

lrg tested major windows and browsed around on dual CPU machine for about 3
hours and didn't find any problems (other than generally sluggish performance).