Open Bug 17191 Opened 20 years ago Updated 8 months ago
text content object is inefficient
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...
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.
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.
Moving to m16
Target Milestone: M15 → M16
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?
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 firstname.lastname@example.org
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.
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.