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

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
2 years 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)

(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)
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: nobody → jfkthame
Status: NEW → ASSIGNED
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)
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)
Attachment #8891823 - Attachment is obsolete: true
Attachment #8891823 - Flags: review?(dbaron)
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+
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
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.