Bug 55005

Summary: SegmentedString operator= always causes a malloc
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED LATER    
Severity: Normal CC: abarth, aestes, darin, mjs, sam, tonyg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 55032    
Bug Blocks: 48719    
Attachments:
Description Flags
attempt, breaks browsing
none
works, i'm not sure it's better none

Eric Seidel (no email)
Reported 2011-02-22 15:22:02 PST
SegmentedString operator= always causes a malloc This is slowing down parsing due to: #0 0x1007beb91 in WTF::fastMalloc at FastMalloc.cpp:3835 #1 0x10184fd29 in VectorBuffer [inlined] at Vector.h:288 #2 0x10184fd29 in VectorBuffer [inlined] at Vector.h:311 #3 0x10184fd29 in Deque [inlined] at Vector.h:363 #4 0x10184fd29 in Deque [inlined] at Deque.h:337 #5 0x10184fd29 in WTF::Deque<WebCore::SegmentedSubstring>::operator= at Deque.h:321 #6 0x10184fd29 in WebCore::SegmentedString::operator= at Vector.h:3835 #7 0x101176a8c in WebCore::SegmentedString::numberOfCharactersConsumed at SegmentedString.h:38 #8 0x101176a8c in WebCore::HTMLSourceTracker::start at HTMLSourceTracker.cpp:3835 The root of the problem seems to be that SegmentedString uses a Deque (m_substrings) which uses a Vector with inlineCapacity=0. I'm investigating making that inlineCapacity=1 and see if the malloc goes away.
Attachments
attempt, breaks browsing (27.37 KB, patch)
2011-02-22 17:40 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
works, i'm not sure it's better (29.56 KB, patch)
2011-02-23 02:52 PST, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2011-02-22 15:24:50 PST
That backtrace is slightly off (we don't go through numberOfCharactersConsumed) due to it being a release build. But the rest is close enough for you to see the point. Anytime someone does: m_segString = otherSegString we do a malloc. Which authors don't always seem to realize (certainly abarth didn't when writing HTMLSourceTracker::start)
Eric Seidel (no email)
Comment 2 2011-02-22 15:50:21 PST
Entertainingly, changing the Buffer in Deque.h to default to inlineCapacity=1 instead of 0 (and rebuilding WebKit) just causes all loads to hang. (I changed all Deque's to use inlineCapacity=1 on their Buffer to see if that would fix my malloc trouble, w/o me having to plumb inlineCapacity support all the way through Deque out to where SegmentedString could specify an inlineCapacity.) WebKit seems to sit there idle. The only thread with anything interesting on it, is the IconDatabase thread waiting to sync to the main. All others are just sitting in the runloop. I suspect that some use of a Deque'd object has a side effect of its empty constructor or some such.
Eric Seidel (no email)
Comment 3 2011-02-22 17:40:00 PST
Created attachment 83421 [details] attempt, breaks browsing
Eric Seidel (no email)
Comment 4 2011-02-22 17:43:10 PST
The attached patch breaks browsing (pages will sometimes fail to load correctly). It's unclear why. Maybe I plumbed the inlineCapacity stuff incorrectly. It's also not clear to me if I should have been adding inlineCapacity as a template argument for the Iterators or not.
Eric Seidel (no email)
Comment 5 2011-02-22 17:47:47 PST
I suspect that the Deque code needs to be taught more about the case where it has inlineCapacity. But maybe VectorBuffer is supposed to abstract all that away?
Eric Seidel (no email)
Comment 6 2011-02-23 00:53:09 PST
It turns out our Deque implementation just doesn't work with capacity=1. It silently fails. I've added an ASSERT to document that and fixed the patch to use capacity 2.
Eric Seidel (no email)
Comment 7 2011-02-23 02:28:10 PST
Changing to capacity 2 makes the patch work, but it's actually *slower*. I think much of that may be due to Deque::operator= doing more work than it needs to. I'm still experimenting.
Eric Seidel (no email)
Comment 8 2011-02-23 02:52:14 PST
Created attachment 83462 [details] works, i'm not sure it's better
Eric Seidel (no email)
Comment 9 2011-02-23 03:34:32 PST
I think I'll post the inlineCapacity support separately and get that landed. Then we can look at the actual details for converting SegmentedString to use it or not.
Eric Seidel (no email)
Comment 10 2011-02-23 03:52:34 PST
OK. I've posted the inlineCapacity support patch on bug 55032 and will deal with actually using it or not (as well as possibly improving Deque<T>::operator=) in a separate patch on this bug.
Eric Seidel (no email)
Comment 11 2011-02-23 15:16:45 PST
This is likely not necessary after bug 55091 and bug 55032.
Eric Seidel (no email)
Comment 12 2011-02-23 15:17:34 PST
Oh, I also did some of the cleanup I suggested in FIXMEs in this patch as part of bug 55083.
Eric Seidel (no email)
Comment 13 2013-03-07 04:44:34 PST
We appear to have a new variant of this in bug 111708.
Note You need to log in before you can comment on or make changes to this bug.