856368 - Some nsPresArena improvements

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

1. remove DEBUG_TRACEMALLOC_PRESARENA -- it's not very useful now that we
   have valgrind/ASan annotations and it blocks other improvements
2. get rid of the extra indirection through the mState pointer, we should
   just store that data in the nsPresArena itself

I'm dividing this into several patches for easier review of each step.

Comment 1

[Mats Palmgren (inactive)]

Attachment: Remove DEBUG_TRACEMALLOC_PRESARENA

Comment 2

[Mats Palmgren (inactive)]

Attachment: Merge nsPresArena::mState into nsPresArena itself

Comment 3

[Mats Palmgren (inactive)]

Attachment: (wdiff version) Merge nsPresArena::mState into nsPresArena itself

This is probably easier to review without the white-space changes.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Adjust memory reporter SizeOf methods accordingly

Comment 5

[Mats Palmgren (inactive)]

Attachment: Inline the AllocateBy* and FreeBy* methods into the header

Comment 6

[Mats Palmgren (inactive)]

Attachment: Remove #include of nscore.h

Comment 7

[Mats Palmgren (inactive)]

Attachment: Comment fixes

Comment 8

[Mats Palmgren (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=a1eb793ef0a0

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #0)
> 1. remove DEBUG_TRACEMALLOC_PRESARENA -- it's not very useful now that we
>    have valgrind/ASan annotations and it blocks other improvements

I've used it for other things.  And it really should be trivial to readd support at the end of the patch series, no?

(In reply to Mats Palmgren [:mats] from comment #6)
> Created attachment 731574 [details] [diff] [review]
> Remove #include of nscore.h

NS_HIDDEN makes non-virtual functions substantially faster in the cases where it kicks in (since on some platforms non-virtual functions that are exposed across library boundaries are performance-wise like virtual functions).  Is this really unused?  (Did we solve that problem globally with libxul?)

Comment 10

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #9)
> I've used it for other things.

Could you describe what other things?

> And it really should be trivial to readd
> support at the end of the patch series, no?

Yes, but I have further improvements in mind and I think
DEBUG_TRACEMALLOC_PRESARENA would make those more complicated.

My idea is to make all pres arenas aligned on a page boundary so that
you can take any arena allocated object and find the arena header with
a bit mask on its address, then store mPresContext/Shell there for fast
access, replacing the slow indirection we do through
nsIFrame->mStyleContext->mRuleNode->mPresContext[->mPresShell]
(and, maybe get rid of mPresContext on the rule node?)

> NS_HIDDEN makes non-virtual functions substantially faster in the cases
> where it kicks in.

OK, I'll keep them.

Comment 11

[Mats Palmgren (inactive)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #9)
> NS_HIDDEN makes non-virtual functions substantially faster in the cases
> where it kicks in

All these methods are now defined in the class though, eg:

  void FreeByFrameID(nsQueryFrame::FrameIID aID, void* aPtr)
  {
    Free(aID, aPtr);
  }

I'd be very surprised if the compiler didn't inline that at the call site.
I guess NS_HIDDEN could help a bit with link time/size though.

If NS_HIDDEN makes the above faster *to call* then we have a lot of room
to optimize our code ;-)

Comment 12

[Mats Palmgren (inactive)]

Waldo, is there a reason mfbt/Attributes.h doesn't have a MOZ_HIDDEN ?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #10)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #9)
> > I've used it for other things.
> 
> Could you describe what other things?

I don't remember, but I'm reasonably confident I have.

> > And it really should be trivial to readd
> > support at the end of the patch series, no?
> 
> Yes, but I have further improvements in mind and I think
> DEBUG_TRACEMALLOC_PRESARENA would make those more complicated.
> 
> My idea is to make all pres arenas aligned on a page boundary so that
> you can take any arena allocated object and find the arena header with
> a bit mask on its address, then store mPresContext/Shell there for fast
> access, replacing the slow indirection we do through
> nsIFrame->mStyleContext->mRuleNode->mPresContext[->mPresShell]
> (and, maybe get rid of mPresContext on the rule node?)

OK, that sounds reasonable.  Though it still should be trivial to readd at the end (by just bloating allocations with the prescontext pointer at the beginning).

Comment 14

[Jeff Walden [:Waldo]]

(In reply to Mats Palmgren [:mats] from comment #12)
> Waldo, is there a reason mfbt/Attributes.h doesn't have a MOZ_HIDDEN ?

No particular reason.  (And needinfo? is probably desirable in addition to CCing me, just in case I miss the bugmail.)

Comment 15

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/339911b031de
https://hg.mozilla.org/integration/mozilla-inbound/rev/c30972306259
https://hg.mozilla.org/integration/mozilla-inbound/rev/88f523148180
https://hg.mozilla.org/integration/mozilla-inbound/rev/852f34e11e00
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da222e62d5f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/339911b031de
https://hg.mozilla.org/mozilla-central/rev/c30972306259
https://hg.mozilla.org/mozilla-central/rev/88f523148180
https://hg.mozilla.org/mozilla-central/rev/852f34e11e00
https://hg.mozilla.org/mozilla-central/rev/7da222e62d5f