357426 - Debug nsAutoLocks cause CPU spike

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[John Gaunt (redfive)]

( copied w/edit from bug 58904 )
We see in Songbird, in debug mode a major thrashing of the OrderTable. We currently use nsAutoLocks in our db code and make a TON of queries. The symptom we see is CPU spiking ( 50% on a 2 CPU machine meaning one cpu is always pegged ) and then the app becomes very un-responsive. Of course in release this symptom goes away.

I have been able to make the symptom go away a couple of ways, 1) change
nsAutoLock back to PRLock/PRUnlock (duh) 2) hack nsAutoLock to have static
methods as does nsAutoMonitor - NewLock() and DestroyLock() that operate
identically to the nsAutoMonitor calls.

I have a patch for 2) that I'll be attaching.

Comment 1

[John Gaunt (redfive)]

Attachment: add NewLock() DestroyLock statics, 1_8 BRANCH

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 242901 [details] [diff] [review]
add NewLock() DestroyLock statics, 1_8 BRANCH

Since you're here, how about static functions to consolidate the #ifdef DEBUG code in NewLock and NewMonitor?  Thanks,

/be

Comment 3

[John Gaunt (redfive)]

Attachment: moved NewLock/DLock out of debug section and added another static

Had to move the NewLock/DestroyLock methods out of the big ifdef DEBUG section (whoops). Added the method requested.

Patch applies cleanly to trunk and branch.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 242949 [details] [diff] [review]
moved NewLock/DLock out of debug section and added another static

Move the OrderTable null test into OnSemaphoreRecycle and r=me.

/be

Comment 5

[John Gaunt (redfive)]

(In reply to comment #4)
> (From update of attachment 242949 [details] [diff] [review] [edit])
> Move the OrderTable null test into OnSemaphoreRecycle and r=me.
> 
> /be
> 

Oh yeah, duh. Needs to be there. I was trying to save the call when there wasn't an OrderTable, but that really doesn't make any sense.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 242949 [details] [diff] [review]
moved NewLock/DLock out of debug section and added another static

sr=jst with brendan's issue fixed before checking in.

Comment 7

[John Gaunt (redfive)]

Checked in 10/26 with brendan's last comment 

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 242949 [details] [diff] [review]
moved NewLock/DLock out of debug section and added another static

approved for 1.8 branch, a=dveditz for drivers

Comment 9

[John Gaunt (redfive)]

Checked in 1.8 branch 11/30

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 242949 [details] [diff] [review]
moved NewLock/DLock out of debug section and added another static

Thougth I stamped this; sorry.

/be