17191 - text content object is inefficient

Status: RESOLVED INACTIVE

(Core :: DOM: Core & HTML, defect, P5)

Description

[kipp]

In particular, the ReplaceData method always uses malloc when in many cases
realloc could be used.

Comment 1

[kipp]

With vidur's new content sink work, we will sometimes create large text elements
as the data streams in. Currently we always malloc and copy when we could be
doing a realloc 99% of the time...

Comment 2

[leger]

Updating to default Editor Assignee...kipp no longer with us :-(

Comment 3

[buster]

mass moving all Kipp's pre-beta bugs to M15.  Nisheeth and I will
prioritize these and selectively move high-priority bugs into M13 and M14.

Comment 4

[buster]

sucker

Comment 5

[Peter Trudelle]

How much overall page-load time is wasted by the unnecessary allocation & copy? 
 Seems like this could sometimes be very slow on Mac OS.  Do you think this 
bug should be on beta1 radar?

Comment 6

[Chris Waterson]

I was talking to beard the other night, and he said that realloc() is, more 
often than not, an anti-optimization. We should examine this one carefully 
before charging off and doing anything...

Comment 7

[rickg]

I'm planning to switch text handling to nsString, which outperforms the existing 
impl by many times.

Comment 8

[rickg]

Moving to m16

Comment 9

[rickg]

Moving out.

Comment 10

[rickg]

This bug has been marked "future" because the original netscape engineer 
working on this is over-burdened. If you feel this is an error, that you or 
another known resource will be working on this bug,or if it blocks your work 
in some way -- please attach your concern to the bug for reconsideration. 

Comment 11

[Adam D. Moss]

rickg, did the nsString switch happen?  Is this
bug dead?

Comment 12

[Christopher Hoess (gone)]

->nobody

Comment 13

[Boris Zbarsky [:bzbarsky]]

This has not happened.  Reassigning to "DOM Other", but we really need a
"Content Model" component....

harish? sicking?  peterv?  jst?  What do you think?  Is there a perf issue here?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think there's a noticable perf-problem here. During loading we only
change the value once (i.e. when initially setting it) so most of the time this
doesn't matter. Of course it wouldn't hurt to fix this bug anyway

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yeah, I just read through that code and if nothing else it looks a lot more
complicated than it really should be.... do you have time to work on this,
sicking?  Or should I try to take a crack at it?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I won't have time to do this anytime soon so please have a go

Comment 17

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This bug is mostly fixed at this point. The only case where we do extra reallocs is when existing data is modified. Appending and setting data is as optimal as can be. Of course, this means that using editor to edit the middle of a textnode is still inefficient, but normal pageload should not be affected.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Appending and setting data is as optimal as can be.

I don't think that's true.  In particular this problem from comment 0:

> the ReplaceData method always uses malloc when in many cases realloc could be
> used.

Is alive and well -- that's exactly what nsTextFragment::SetTo (used by nsGenericDOMDataNode::SetText) does.  Now nsTextFragment::Append _does_ realloc...  But nsGenericDOMDataNode::ReplaceData is even worse -- it allocates a new buffer, copies the old text into it, copies the new text into it, then makes _another_ copy of it for the text fragment.

I agree that pageload is probably not affected much by either of these, but they're still pretty suboptimal.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > the ReplaceData method always uses malloc when in many cases realloc could 
> > be used.
> 
> Is alive and well -- that's exactly what nsTextFragment::SetTo (used by
> nsGenericDOMDataNode::SetText) does. Now nsTextFragment::Append _does_
> realloc...  

Ah, I see what you mean, we should reuse the existing buffer in nsTextFragment::SetText if possible. That I agree with.

> But nsGenericDOMDataNode::ReplaceData is even worse -- it 
> allocates a new buffer, copies the old text into it, copies the new text into 
> it, then makes _another_ copy of it for the text fragment.

Well.. nsTextFragment is what makes the final copy. But yeah, the extra copy that nsGenericDOMDataNode makes is wasted.

The way to fix this would be to give nsTextFragment methods for replacing data in the middle of the buffer. That way it could check for 8-bit-ness of its internal buffer and the data that's coming in and do the minimal amount of allocation/copying.

However since the only users of these methods are the DOM methods InsertData/ReplaceData/DeleteData I wonder if it's worth the hassle. I guess it may not be *that* much extra code if we share codepath with the existing nsTextFragment::Append function.

Comment 21

[Firefox Bug Husbandry Bot]

Bulk priority change, per :mdaly