[webkit-reviews] review denied: [Bug 43987] Downloading using XHR is much slower than before : [Attachment 69648] Proposed patch, first stab

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Andreas Kling
<kling at webkit.org>'s request for review:
Bug 43987: Downloading using XHR is much slower than before
https://bugs.webkit.org/show_bug.cgi?id=43987

Attachment 69648: Proposed patch, first stab
https://bugs.webkit.org/attachment.cgi?id=69648&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
Okay, I don't think this is quite how we want to fix this.  Fundamentally this
patch is doing the right thing, in that we should be using a smarter string
building layer rather that resolving a fresh string every time new data is
available, but it's doing it in slightly the wrong way:

(1) I think this patch is fixing the problem in the wrong place.  We don't want
to be adding more code to ScriptString - in fact, we don't really want
ScriptString to exist at all! (this class only existed because XMLHttpRequest
used to hold a UString, which wasn't great from design perspective in the first
place, and clearly isn't ).  Strings in WebCore are by design immutable, so
XMLHttpRequest holding onto a string and appending to it is not the correct
approach.  The place this problem needs to be fixed is in XMLHttpRequest. 
(Moreover, all uses of ScriptString are now wrong.  XMLHttpRequest only held a
StringString under the assumption this can be used for fast concatenation,
which is no longer (necessarily) the case – as demonstrated by this bug.  New
code in fileapi is currently using ScriptString to mimic XMLHttpRequest, and as
such will also be mimicking this O(N^2) performance too.  Whatever fix is made
for this bug should also be applied to FileReader/FileReaderSync - and having
done so StringString will be redundant.)

(2) We shouldn't be adding a new type to build Strings – we already have
StringBuilder.	XMLHttpRequest should just use a StringBuilder.  If
StringBuilder does not have the performance characteristics we require, we
should fix it ...

(3) ... and StringBuilder probably doesn't have the performance characteristics
we require. :-)  Building the string up as a rope saves resolving the string on
every append, but accesses to the responseText string while data is being
loaded will still result in a copy to resolve, and as such repeated calls to
access the responseText could still result in repeated copying of the character
data in the String.  Instead we should probably look at using a allocating a
string buffer with overcapacity, such that appends can write into the existing
buffer.  We should be able support with using the existing createUninitialized
& substring mechanisms supported by WTF StringImpls.  Using a 2x overcapacity
should make string construction O(N), with zero cost to resolve.

I have a patch for (1) and (2), will attach, and most of the code written for
(3) too.

G.