1367209 - nsCounterManager::DestroyNodesFor does an unconditional hashtable iteration on every frame destruction

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

It's called from nsCSSFrameConstructor::NotifyDestroyingFrame.

We should consider having a flag on the frame that says that it _might_ be found in the counter manager.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that the call to nsGenConList::DestroyNodesFor in nsCSSFrameConstructor::NotifyDestroyingFrame is already guarded on a flag.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - Fix some indentation and code style issues

This file is a mess of different indentation and coding styles, 
let's just fix that once and for all.

Comment 3

[Mats Palmgren (inactive)]

Attachment: wdiff of part 1 (for easier reviewing)

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 2 - Remove a useless constructor

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 3 - Add a generic frame state bit, NS_FRAME_HAS_CSS_COUNTERS, to indicate that a frame maybe has some counter styles and thus be known by nsCounterManager

This uses up the last remaining global frame bit, but I have patches
to convert some private bits to global to compensate for this.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 4 - Use LookupForAdd to avoid an additional hashtable lookup when inserting a new entry

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ed9078e762e41d09eb1d933bb4058e90d4ce8ad

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Mats Palmgren (:mats) from comment #5)
> This uses up the last remaining global frame bit, but I have patches
> to convert some private bits to global to compensate for this.

I'm guessing you probably meant to say "convert some global bits to private" here.

Comment 8

[Mats Palmgren (inactive)]

> I'm guessing you probably meant to say "convert some global bits to private" here.

No, I intend to move some of the private bits for "Block" and "Text" groups (which
are currently full) to bool:1 on their respective classes instead.  This makes two
bits in the private range unused, so they can be reclaimed as global bits instead.
I intend to convert more eventually, because I think it's a more efficient use of
the frame bits; currently there are many frame types that use none or only a few
of the reserved private bits.  I'll post the patches later...

Comment 9

[Jonathan Kew [:jfkthame]]

Ah, I see. Yes, that makes sense.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1005b7702f1e
part 1 - Fix some indentation and code style issues.  r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbfa99d9a737
part 2 - Remove a useless constructor.  r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9d1468f7b4
part 3 - Add a generic frame state bit, NS_FRAME_HAS_CSS_COUNTERS, to indicate that a frame maybe has some counter styles and thus be known by nsCounterManager.  r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/22108a33acd5
part 4 - Use LookupForAdd to avoid an additional hashtable lookup when inserting a new entry.  r=jfkthame

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1005b7702f1e
https://hg.mozilla.org/mozilla-central/rev/fbfa99d9a737
https://hg.mozilla.org/mozilla-central/rev/1b9d1468f7b4
https://hg.mozilla.org/mozilla-central/rev/22108a33acd5

Comment 12

[Boris Zbarsky [:bzbarsky]]

Note that frames have free bool:1 bits as well.  See mMayHaveRoundedCorners and the following comment about there being space for 15 more bool:1 there.