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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bzbarsky, Assigned: mats)

Tracking

({perf})

53 Branch
mozilla55
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(5 attachments)

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.
Note that the call to nsGenConList::DestroyNodesFor in nsCSSFrameConstructor::NotifyDestroyingFrame is already guarded on a flag.
(Reporter)

Updated

a year ago
Whiteboard: [qf]
Whiteboard: [qf] → [qf:p1]
(Assignee)

Updated

a year ago
Assignee: nobody → mats
(Assignee)

Comment 2

a year ago
Created attachment 8874196 [details] [diff] [review]
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.
Attachment #8874196 - Flags: review?(jfkthame)
(Assignee)

Comment 3

a year ago
Created attachment 8874197 [details] [diff] [review]
wdiff of part 1 (for easier reviewing)
(Assignee)

Comment 4

a year ago
Created attachment 8874198 [details] [diff] [review]
part 2 - Remove a useless constructor
Attachment #8874198 - Flags: review?(jfkthame)
(Assignee)

Comment 5

a year ago
Created attachment 8874199 [details] [diff] [review]
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.
Attachment #8874199 - Flags: review?(jfkthame)
(Assignee)

Comment 6

a year ago
Created attachment 8874200 [details] [diff] [review]
part 4 - Use LookupForAdd to avoid an additional hashtable lookup when inserting a new entry

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ed9078e762e41d09eb1d933bb4058e90d4ce8ad
Attachment #8874200 - Flags: review?(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.
Attachment #8874196 - Flags: review?(jfkthame) → review+
Attachment #8874198 - Flags: review?(jfkthame) → review+
Attachment #8874199 - Flags: review?(jfkthame) → review+
Attachment #8874200 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 8

a year ago
> 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...
Ah, I see. Yes, that makes sense.

Comment 10

a year ago
Pushed by mpalmgren@mozilla.com:
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
(Assignee)

Updated

a year ago
Flags: in-testsuite-
(Assignee)

Updated

a year ago
Keywords: perf
OS: Unspecified → All
Hardware: Unspecified → All
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.
You need to log in before you can comment on or make changes to this bug.