Remove nsNodeInfo::sNodeInfoPool and use vanilla allocation for nsNodeInfo objects.

RESOLVED FIXED in mozilla22

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
Attachment #720466 - Flags: review?(bugs)
(Assignee)

Updated

4 years ago
Blocks: 847248

Comment 2

4 years ago
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+
(Assignee)

Comment 3

4 years ago
> 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

4 years ago
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.
(Assignee)

Comment 5

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a453f499b2
(Assignee)

Updated

4 years ago
Whiteboard: [MemShrink]
https://hg.mozilla.org/mozilla-central/rev/c5a453f499b2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.