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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

2 years ago
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.
Reporter

Comment 1

2 years ago
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

2 years ago
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

2 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Updated

2 years 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

2 years 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

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

Comment 6

2 years ago
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

2 years 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

2 years 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

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