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.
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → mozilla1.5alpha
over to me.
Assignee: darin → dougt
Status: ASSIGNED → NEW
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.
Created attachment 123075 [details] [diff] [review] test patch. Not to be checked in as-is. Use only for testing/benchmarking.
Created attachment 123078 [details] [diff] [review] second patch. no race in the sync-current-thread case.
Attachment #123075 - Attachment is obsolete: true
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.
Attachment #123075 - Flags: superreview+
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).
brendan, would you like to consider this change for 1.4?
> 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?
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!
I see no difference in VmRSS usage.
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?
Is there a leak?
no leak before or after.
Is bug 203596 the leak?
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.
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 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.
Attachment #123078 - Flags: approval1.4? → approval1.4-
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.
why bother, i guess.
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
mozilla1.4 shipped. unsetting blocking1.4 request.
You need to log in before you can comment on or make changes to this bug.