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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nicholas Nethercote [inactive]]

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 [inactive]]

Attachment: 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][[e-mail]]]

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 [inactive]]

> 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][[e-mail]]]

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 [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a453f499b2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c5a453f499b2

Comment 7

[Intermittent Failures Robot]

1 failures in 3276 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 1

Platform and build breakdown:

• toolchains: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=847210&startday=2023-07-31&endday=2023-08-06&tree=all