nsTextFrame::CharacterDataChanged() uses content properties that are expensive to access

RESOLVED FIXED in Firefox 57

Status

()

Core
Layout
RESOLVED FIXED
21 days ago
11 days ago

People

(Reporter: Ehsan, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

https://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/generic/nsTextFrame.cpp#4824

Accessing these properties requires a linked list traversal and a hashtable lookup which is no good.  It would be nice if these were cached on nsGenericDOMDataNode instead.
David, does this plan make sense?  Are nsGenericDOMDataNode's the only kind of node we may deal with here?
Flags: needinfo?(dbaron)
Maybe Jonathan can help here?  If not, I might have a chance to look next week...
Flags: needinfo?(jfkthame)
(Assignee)

Comment 3

19 days ago
Created attachment 8891823 [details] [diff] [review]
Cache properties needed by nsTextFrame directly in the nsGenericDOMDataNode instead of in separate properties hashtable

Looks like we can eliminate the hashtable usage here by storing the NewlineProperty and FlowLengthProperty directly in nsGenericDOMDataNode, as suggested in comment 0. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=870cceb93b2485a36d358ad06b39ba57cce65e13.
Attachment #8891823 - Flags: review?(dbaron)
(Assignee)

Updated

19 days ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Updated

19 days ago
Flags: needinfo?(jfkthame)
What does the overall size change here look like?  This looks like it's adding 128 bits to DOM text and comment nodes.  What percentage of the time were we storing the properties before the patch?  Is this like a 128 bit increase in the size, or were those properties around for a long time?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 5

18 days ago
Hmm, you're right to be suspicious here; I just did a little instrumentation and confirmed that these properties are relatively rare, so I don't think we should give them this much space. Let's try an alternative...
Flags: needinfo?(jfkthame)
(Assignee)

Updated

18 days ago
Attachment #8891823 - Attachment is obsolete: true
Attachment #8891823 - Flags: review?(dbaron)
(Assignee)

Comment 6

18 days ago
Created attachment 8892092 [details] [diff] [review]
Use flag bits in the nsGenericDOMDataNode to record whether nsTextFrame-related properties are present, so we don't perform unnecessary hashtable lookups in CharacterDataChanged etc

Fortunately, there seem to be a number of spare flag bits in nsGenericDOMDataNode, so we can do something like this to hide the hashtable accesses behind a much cheaper flag bit test.
Attachment #8892092 - Flags: review?(dbaron)
Comment on attachment 8892092 [details] [diff] [review]
Use flag bits in the nsGenericDOMDataNode to record whether nsTextFrame-related properties are present, so we don't perform unnecessary hashtable lookups in CharacterDataChanged etc

>+  if (PresContext()->BidiEnabled() &&
>+      aContent->HasFlag(NS_HAS_FLOWLENGTH_PROPERTY)) {
>     aContent->DeleteProperty(nsGkAtoms::flowlength);
>+    aContent->UnsetFlags(NS_HAS_FLOWLENGTH_PROPERTY);
>   }

The BidiEnabled() test seems like it would just slow things down at this point; I think it makes sense to drop it (twice).


Also, please guard the DeleteProperty calls in SetNextContinuation and SetNextInFlow in nsTextFrame.h with the bit test, and remove the bit.

r=dbaron with those two (four) things fixed
Flags: needinfo?(dbaron)
Attachment #8892092 - Flags: review?(dbaron) → review+
(Assignee)

Comment 8

11 days ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9529eb7b4087b687cc2fbb362a56b54464d9abf3
Bug 1385395 - Use flag bits in the nsGenericDOMDataNode to record whether nsTextFrame-related properties are present, so we don't perform unnecessary hashtable lookups in CharacterDataChanged etc. r=dbaron

Comment 9

11 days ago
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9529eb7b4087
Use flag bits in the nsGenericDOMDataNode to record whether nsTextFrame-related properties are present, so we don't perform unnecessary hashtable lookups in CharacterDataChanged etc. r=dbaron

Comment 10

11 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9529eb7b4087
Status: ASSIGNED → RESOLVED
Last Resolved: 11 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.