(cf. bug 77540.) nsTextNode ends up relying on the nsGenericDOMDataNode's implementation of AppendData(), which fights with nsTextNode's attempts to keep data stored as single-byte when possible.
Keywords: footprint, mozilla0.9.1, perf
OS: Windows 2000 → All
Hardware: PC → All
Waterson, what do you suggest we do about this?
Target Milestone: --- → mozilla1.0
The specific problem is that whenever the content sink calls AppendData() on a text node (instead of SetText(), e.g.), we end up having a worst-case scenario where the existing single-byte data is needlessly re-inflated to double-byte, forcing a re-allocation of the buffer. IIRC, bug 77540 highlighted this: it was a long flat text doc that caused us to oscillate back and forth between SetText() and AppendData() due to the buffer size in the content sink. This ended up causing us to inflate and deflate the text repeatedly. I guess I'm proposing that nsTextNode should implement AppendData() itself, instead of forwarding to mInner. (nsTextNode could probably make use of some of the new string fu, too.) Alternatively, we could - Give up on storing data as ASCII, and just use nsGenericDOMDataNode's storage management. This would sort of suck, because we'd have to deflate to deflate to measure text in nsTextFrame. - Move the ASCII handling out of nsTextNode into nsGenericDOMDataNode. I think I might like the latter alternative best of all...
jst, any interest in doing this (soon)? If not, please reassign back to me and I'll do it.
Chris, feel free to work on this, I got my plate more than full at the moment...
Assignee: jst → waterson
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: mozilla1.0 → mozilla0.9.7
Target Milestone: mozilla0.9.8 → Future
Created attachment 73179 [details] [diff] [review] patch for nsTextNode to implement its own AppendData() As Chris's suggestion, the existing single-byte data is no longer needlessly re-inflated to double-byte.
Jeff, in stead of adding an AppendData() method to nsTextNode that is more efficient than the one in nsGenericDOMDataNode, why not simply speed up the one in nsGenericDOMDataNode? I.e. take the code you wrote in nsTextFragment and move it into nsGenericDOMDataNode::AppendData(const nsAString&). Let me know if you don't have time to work on that and I'll take over (unless someone else volunteers).
Created attachment 78515 [details] [diff] [review] As jst's suggestion, do it in nsGenericDOMDataNode::AppendData Thank you, jst. Anyone who has the right, please obsolete patch 73179.
Attachment #73179 - Attachment is obsolete: true
Comment on attachment 78515 [details] [diff] [review] As jst's suggestion, do it in nsGenericDOMDataNode::AppendData sr=jst
Attachment #78515 - Flags: superreview+
Comment on attachment 78515 [details] [diff] [review] As jst's suggestion, do it in nsGenericDOMDataNode::AppendData + void AppendTo(nsCString& aCString) const; Can you use nsACString? (ideally you'd also change the nsString of the other AppendTo function to nsAString) r=biesi with at least the nsCString->nsACString change
Attachment #78515 - Flags: review+
Unfortunately we can't use nsACString there since that would have performance implications in the implementation of that method. We'd end up having to use NS_Convert* in the implementation which would cost us extra string copying and potentially memory allocation/freeing.
ah yeah... ignore my comment, r=biesi on the current patch
Assignee: waterson → jst
Status: ASSIGNED → NEW
Whiteboard: [HAVE FIX]
Target Milestone: Future → mozilla1.0
Comment on attachment 78515 [details] [diff] [review] As jst's suggestion, do it in nsGenericDOMDataNode::AppendData firstname.lastname@example.org for drivers. Please check into both branch and trunk.
Attachment #78515 - Flags: approval+
Fixed on the trunk.
Status: NEW → ASSIGNED
The checkin had an interesting (beneficial) side effect as discovered by email@example.com: bug 121841 is almost gone. It is one of the most duped hang bugs we have. Note that this patch does not fix it, this merely makes it less likely to happen and hides it in most cases, it seems.
Comment 13 shows drivers approval on Trunk & Branch, so why has this made it into Trunk only?
Us Netscape people need even more approval before checking in on the branch, approval request sent...
It's probably worth explicitly commenting on the naked nsCString usage: I would have made the same mistake that biesi did. (And we don't want someone to `clean this up' later.)
This patch has lived on the trunk and gotten some testing there already, this change affects any page with text chunks that are more than 4096 characters long, i.e. most plain text files out there. If there'd be problems with this patch I would've expected to see them really early, and none have shown up so far.
Comment added on the trunk.
adt1.0.0+ on behalf of the adt. Please check into the branch today and add fixed1.0.0 in the keyword field.
Keywords: adt1.0.0 → adt1.0.0+
Fixed on the 1.0 branch too.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Johnney is there any testcase I can use to test this ? or could you please verify this one ? thnx.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.