204962 - PLEvent should lazily allocate lock and cvar

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Darin Fisher]

PLEvent should lazily allocate lock and cvar.  The lock and cvar allocated in
PL_InitEvent is only ever used by PL_PostSynchronousEvent.  Since so many of our
events in mozilla are asynchronous, this means we needlessly allocate locks and
cvars a bunch.  cathleen has (or had) some spacetrace data indicating a large
number of allocations resulting from this.

Comment 1

[Doug Turner (:dougt)]

over to me.

Comment 2

[Doug Turner (:dougt)]

.

Comment 3

[Cathleen]

Yes, I have a linux spacetrace log.  normal build, without loading images (so
that image allocations won't over mask others).   It turns out 44% of
allocations comes from html catagory which includes parser, content sink, frame
managers, genericHTMLElements and more.   Anyway, within top 20 uniqe allocation
stacks in the html catatory, 12 of them showed that allocations being held by
PL_InitEvent, which mounts up to 5202 objects.  Not to mention the rest of
allocation callsites. 

Comment 4

[Doug Turner (:dougt)]

Attachment: test patch.

Not to be checked in as-is.  Use only for testing/benchmarking.

Comment 5

[Doug Turner (:dougt)]

Attachment: second patch.

no race in the sync-current-thread case.

Comment 6

[Darin Fisher]

Comment on attachment 123075 [details] [diff] [review]
test patch.

>+        // we will die here if a RevokeEvent occurs from thread B while processing
>+        // a short curcuit sync event on thread A.

not true since short circuit sync event is never actually posted to a queue.

sr=darin with that comment removed.

Comment 7

[Cathleen]

patch does work.

I no longer see allocations being held by PL_InitEvent.  None of the top 500
unique allocation callsties show it.

Also, I see the heap gowth went from 2.78MB down to 1.95MB ( 2 cycles of
cowtools page load).

Comment 8

[Doug Turner (:dougt)]

brendan, would you like to consider this change for 1.4?  

Comment 9

[Darin Fisher]

> Also, I see the heap gowth went from 2.78MB down to 1.95MB ( 2 cycles of
> cowtools page load).

that's huge!  are you measuring that with spacetrace?  what platform?  is this
after running cowtools and then letting things sit for a while?

Comment 10

[Cathleen]

yes, data from spacetrace.  on linux.  
running cowtool pageload for 2 cycles then load a blank page and sit for a wile.  

i guess, with this patch, we're not holding onto many thousands of allocations
for a long period of time. ;-)  it's a good thing!

Comment 11

[Doug Turner (:dougt)]

I see no difference in VmRSS usage.

Comment 12

[Cathleen]

spacetrace stacks showing allocations to do with malloc, calloc, realloc
specific to our app.  I think VMRSS does include all sys lib loaded by us?

Comment 13

[Asa Dotzler [:asa]]

Is there a leak?

Comment 14

[Doug Turner (:dougt)]

no leak before or after.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is bug 203596 the leak?

Comment 16

[Doug Turner (:dougt)]

Although these two bugs are indepedent, the footprint savings could have been
caused by byt the other bug.  We should check these patches out indepedently to
verify.

Comment 17

[Cathleen]

I'll update my tree to get patch from bug 203596, then get an updated spacetrace
data to see if the problem goes away without the patch in this bug.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 123078 [details] [diff] [review]
second patch.

It sounds like this isn't going to do much for us now that the leak is fixed,
so I don't see any reason to take the risk for 1.4.  If there's new
information, please renominate.

Comment 19

[Doug Turner (:dougt)]

Checking in plevent.c;
/cvsroot/mozilla/xpcom/threads/plevent.c,v  <--  plevent.c
new revision: 1.38; previous revision: 1.37
done

We should reconsider this fix for the 1.4 branch.

Comment 20

[Doug Turner (:dougt)]

why bother, i guess.

Comment 21

[Asa Dotzler [:asa]]

mozilla1.4 shipped. unsetting blocking1.4 request.