[webkit-reviews] review canceled: [Bug 103295] TextIterator unnecessarily converts 8 bit strings to 16 bits : [Attachment 176103] Patch

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has canceled Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 103295: TextIterator unnecessarily converts 8 bit strings to 16 bits
https://bugs.webkit.org/show_bug.cgi?id=103295

Attachment 176103: Patch
https://bugs.webkit.org/attachment.cgi?id=176103&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
(In reply to comment #3)
> (From update of attachment 176103 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=176103&action=review
> 
> > Source/WebCore/editing/TextIterator.cpp:468
> > +	 ASSERT(static_cast<int>(index) < length());
> > +	 if (!(static_cast<int>(index) < length()))
> 
> I think the typecast needs to be done in the other direction, casting
length() to an unsigned, otherwise large indexes will get past this check
because they get converted to negative numbers.

Done.

> > Source/WebCore/editing/TextIterator.h:66
> > +#if 0
> >  UChar* plainTextToMallocAllocatedBuffer(const Range*, unsigned&
bufferLength, bool isDisplayString, TextIteratorBehavior =
TextIteratorDefaultBehavior);
> > +#endif
> 
> Why just #if'd out?

Oversight in original patch - fixed.
 
> > Source/WebCore/editing/TextIterator.h:103
> > +	 int startOffset() const { return m_positionStartOffset; }
> > +	 const String string() const { return m_text; }
> > +	 const UChar* characters() const { return m_textCharacters ?
m_textCharacters : m_text.characters() + startOffset(); }
> > +	 UChar characterAt(unsigned index) const;
> > +	 void appendTextToStringBuilder(StringBuilder&) const;
> 
> This additional API makes TextIterator a lot more confusing to use than it
was before. The old API was designed to make it hard to use incorrectly; there
was no copying of text ever. I’m particularly confused about how to use the
string() function. Do all these new functions need to be public?

I made startOffset() and string() private.  The way to access text data is as
before via characters(), if one character is wanted use characterAt() or if
appending to a StringBuilder using appendTextToStringBuilder().  The two new
functions are optimized for 8 bit strings.

> The string() function should not return "const String". It should just return
"String".

Done.

> > Source/WebKit/mac/WebView/WebFrame.mm:500
> > -	 // This will give a system malloc'd buffer that can be turned directly
into an NSString
> > -	 unsigned length;
> > -	 UChar* buf = plainTextToMallocAllocatedBuffer(core(range), length,
true);
> > -	 
> > -	 if (!buf)
> > -	     return [NSString string];
> > -
> > -	 // Transfer buffer ownership to NSString
> > -	 return [[[NSString alloc] initWithCharactersNoCopy:buf length:length
freeWhenDone:YES] autorelease];
> > +	 return plainText(core(range), TextIteratorDefaultBehavior, true);
> 
> Why is it OK to remove the plainTextToMallocAllocatedBuffer optimization?
That was a critical change back when Antti originally did it. Did you research
why it was needed and determine it’s not needed any more?

The original change was done in http://trac.webkit.org/changeset/24832 to fix
fragmentation caused by plainText() on large full documents.  The change did
two things to fix the problem, use system malloc and allocate large buffers. 
The patch I propose goes back to using fastMalloc, but it keeps the large
buffer behavior exactly for the original reason.

I tested it with the test referenced in the code and posted those results with
the original patch.  That is a performance test and not a memory test.	The
test site "http://dscoder.com/test.txt" referenced in Antti's change is no
longer available.  Instead I tried some memory tests using the HTML 5 spec file
in PerformanceTests/Parser/resources/html5.html (a 5.8MB file).  Loading this
file invokes plainText() once to create a 2.6MB String.  I loaded this file and
measured memory before and after this patch:
			  Baseline     With Patch   
Fast Malloc Allocations     835719	   720998
Fast Malloc Dirty	   133.1MB	  127.5MB
System Malloc Allocations    35318	    35320
System Malloc Dirty	    25.2MB	   22.8MB
Total Dirty		   171.0MB	  162.7MB