nsTextNode should implement AppendData()

VERIFIED FIXED in mozilla1.0

Status

()

Core
DOM: Core & HTML
P3
normal
VERIFIED FIXED
17 years ago
4 years ago

People

(Reporter: Chris Waterson, Assigned: jst)

Tracking

({helpwanted, memory-footprint, perf})

Trunk
mozilla1.0
helpwanted, memory-footprint, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [HAVE FIX])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

17 years ago
(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.
(Reporter)

Updated

17 years ago
Keywords: footprint, mozilla0.9.1, perf
OS: Windows 2000 → All
Hardware: PC → All
(Assignee)

Comment 1

17 years ago
Waterson, what do you suggest we do about this?
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 2

17 years ago
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...

Updated

17 years ago
QA Contact: lchiang → stummala
(Reporter)

Comment 3

17 years ago
jst, any interest in doing this (soon)? If not, please reassign back to me and
I'll do it.
(Reporter)

Updated

17 years ago
Blocks: 104669
(Assignee)

Comment 4

17 years ago
Chris, feel free to work on this, I got my plate more than full at the moment...
Assignee: jst → waterson
(Reporter)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: mozilla1.0 → mozilla0.9.7
(Reporter)

Updated

17 years ago
Target Milestone: mozilla0.9.7 → mozilla0.9.8
(Reporter)

Updated

17 years ago
Keywords: helpwanted
Target Milestone: mozilla0.9.8 → Future

Updated

17 years ago
Keywords: mozilla1.0+

Comment 5

17 years ago
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.
(Assignee)

Comment 6

16 years ago
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).

Comment 7

16 years ago
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.
(Assignee)

Updated

16 years ago
Attachment #73179 - Attachment is obsolete: true
(Assignee)

Comment 8

16 years ago
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+
(Assignee)

Comment 10

16 years ago
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)

Comment 12

16 years ago
Taking bug.
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

a=rjesup@wgate.com for drivers.  Please check into both branch and trunk.
Attachment #78515 - Flags: approval+
(Assignee)

Comment 14

16 years ago
Fixed on the trunk.
Status: NEW → ASSIGNED
The checkin had an interesting (beneficial) side effect as discovered by
ajschult@eos.ncsu.edu: 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 16

16 years ago
Comment 13 shows drivers approval on Trunk & Branch, so why has this made it
into Trunk only?
(Assignee)

Updated

16 years ago
Keywords: adt1.0.0
(Assignee)

Comment 17

16 years ago
Us Netscape people need even more approval before checking in on the branch,
approval request sent...
(Reporter)

Comment 18

16 years ago
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.)
(Assignee)

Comment 19

16 years ago
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.
(Assignee)

Comment 20

16 years ago
Comment added on the trunk.

Comment 21

16 years ago
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+
(Assignee)

Comment 22

16 years ago
Fixed on the 1.0 branch too.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Keywords: fixed1.0.0
Resolution: --- → FIXED

Comment 23

16 years ago
Johnney is there any testcase I can use to test this ?
or could you please verify this one ?
thnx.
(Assignee)

Comment 24

16 years ago
Verified.
Status: RESOLVED → VERIFIED
Keywords: verified1.0.0

Updated

8 years ago
Component: DOM: Abstract Schemas → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.