Last Comment Bug 847210 - Remove nsNodeInfo::sNodeInfoPool and use vanilla allocation for nsNodeInfo objects.
: Remove nsNodeInfo::sNodeInfoPool and use vanilla allocation for nsNodeInfo ob...
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on:
Blocks: 847248
  Show dependency treegraph
 
Reported: 2013-03-03 13:44 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2013-03-05 07:43 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove nsNodeInfo::sNodeInfoPool and use vanilla allocation for nsNodeInfo objects. (16.10 KB, patch)
2013-03-03 13:45 PST, Nicholas Nethercote [:njn] (on vacation until July 11)
bugs: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-03 13:44:48 PST
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.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-03 13:45:01 PST
Created attachment 720466 [details] [diff] [review]
Remove nsNodeInfo::sNodeInfoPool and use vanilla allocation for nsNodeInfo objects.

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.
Comment 2 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-03-04 06:18:16 PST
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.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 14:39:17 PST
> 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);|.
Comment 4 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2013-03-04 14:52:53 PST
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.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2013-03-04 16:55:03 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a453f499b2
Comment 6 Ryan VanderMeulen [:RyanVM] 2013-03-05 07:43:25 PST
https://hg.mozilla.org/mozilla-central/rev/c5a453f499b2

Note You need to log in before you can comment on or make changes to this bug.