Open Bug 17191 Opened 20 years ago Updated 8 months ago

text content object is inefficient

Categories

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

x86
All
defect

Tracking

()

Future

People

(Reporter: kipp, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

In particular, the ReplaceData method always uses malloc when in many cases
realloc could be used.
Status: NEW → ASSIGNED
Summary: text content object is inefficient → [perf] text content object is inefficient
Target Milestone: M14
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...
Assignee: kipp → rickg
Status: ASSIGNED → NEW
Updating to default Editor Assignee...kipp no longer with us :-(
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.
Assignee: kipp → rickg
sucker
Keywords: perf
Summary: [perf] text content object is inefficient → text content object is inefficient
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?
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...
I'm planning to switch text handling to nsString, which outperforms the existing 
impl by many times.
Status: NEW → ASSIGNED
Moving to m16
Target Milestone: M15 → M16
Moving out.
Target Milestone: M16 → M18
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. 
Target Milestone: M18 → Future
rickg, did the nsString switch happen?  Is this
bug dead?
->nobody
Assignee: rickg → nobody
Status: ASSIGNED → NEW
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?
Assignee: nobody → jst
Component: Layout → DOM Other
QA Contact: petersen → gerardok
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
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?
I won't have time to do this anytime soon so please have a go
Mass-reassigning bugs to dom_bugs@netscape.com
Assignee: jst → dom_bugs
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.
> 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.
> > 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.
QA Contact: gerardok → ian
Assignee: general → nobody
QA Contact: ian → general
OS: Linux → All
Bulk priority change, per :mdaly
Priority: P3 → P5
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.