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

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

David, does this plan make sense?  Are nsGenericDOMDataNode's the only kind of node we may deal with here?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe Jonathan can help here?  If not, I might have a chance to look next week...

Comment 3

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 5

[Jonathan Kew [: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...

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[Jonathan Kew [:jfkthame]]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9529eb7b4087