Closed Bug 847210 Opened 9 years ago Closed 9 years ago
Node Info::s Node Info Pool and use vanilla allocation for ns Node Info objects .
nsNodeInfo objects are allocated via nsNodeInfo::sNodeInfoPool, which is an nsFixedSizeAllocator. This is a bad idea. - With a good allocator (like jemalloc) it's usually best to let the allocator do its job, unless you know something special about the lifetimes of the involved objects. In this case, we have no special information. - The nsFixedSizeAllocator never shrinks. After running MemBench (http://gregor-wagner.com/tmp/mem) which opens 150 tabs and then closes them all, the nsFixedSizeAllocator takes up 4 MiB (on Linux64). With vanilla allocation that space would be freed.
This patch removes the pool and allocates nsNodeInfo objects via |new|. I'm not familiar with this code and so I'm just kind of guessing (especially the NS_IMPL_CYCLE_COLLECTING_RELEASE change) but it seems to work ok -- the try run at https://tbpl.mozilla.org/?tree=Try&rev=98917cf652a3 was green. But please check carefully. The patch also removes some unnecessary #includes of nsFixedSizeAllocator.h.
Attachment #720466 - Flags: review?(bugs)
Comment on attachment 720466 [details] [diff] [review] Remove nsNodeInfo::sNodeInfoPool and use vanilla allocation for nsNodeInfo objects. Could you please keep LastRelease() so that mOwnerManager is still kungfuDeathGrip'ed. But just call 'delete this' after that.
Attachment #720466 - Flags: review?(bugs) → review+
> Could you please keep LastRelease() so that > mOwnerManager is still kungfuDeathGrip'ed. > But just call 'delete this' after that. I'm happy to do it, though it doesn't look like it will make any difference, because the last line of ~nsNodeInfo() is |NS_RELEASE(mOwnerManager);|.
There used to be some odd ownership model which required manager to stay alive longer than nodeinfo. Can't remember now, and would be better to change such thing in a different bug.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.