[webkit-reviews] review granted: [Bug 33222] Make text decoders and resource loaders read from segmented SharedBuffer : [Attachment 45965] Make TextResourceDecoder read from segmented SharedBuffer

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yong Li <yong.li.webkit at gmail.com>'s
request for review:
Bug 33222: Make text decoders and resource loaders read from segmented
SharedBuffer
https://bugs.webkit.org/show_bug.cgi?id=33222

Attachment 45965: Make TextResourceDecoder read from segmented SharedBuffer
https://bugs.webkit.org/attachment.cgi?id=45965&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +String TextResourceDecoder::decode(const SharedBuffer& data)
> +{
> +    String result;
> +    const char* segment;
> +    unsigned offset = 0;
> +    while (unsigned length = data.getSomeData(segment, offset)) {
> +	   result += decode(segment, length);
> +	   offset += length;
> +    }
> +    return result;
> +}

It's great factoring to make use a function like this instead of spreading the
code around.

I also note that all the clients who use this function also do the flushing, so
in the future might want to make this helper function include the flush and
have some name like "decodeAll" rather than just being cleanly parallel to the
virtual decode function.

This is still a quite-inefficient algorithm for creating a string out of
multiple decoded chunks; any loop that appends to a String is suboptimal
because making a new String by appending two strings *always* involves
allocating a fresh buffer each time and copying all the data; this inefficiency
might make some real world cases pathologically worse if we find ourselves with
multiple segments. It might actually be more efficient to do the dumb thing the
old code was doing, and concatenate them all, just because the decoded data
would be dealt with all at once even though the encoded data ends up being
copied.

I think at some point we will want to change this. Even now I think we could do
better with Vector<UChar>.

Maybe we want to get rid of the use of String in the interface to decoders
eventually?

I am conflicted about the patch. It seems like a good idea in theory, but in
practice it seems that this patch claiming to speed things up might always end
up slowing things down instead. And might cause us to use more memory too.

> +    String decode(const SharedBuffer& data);

I don't think the argument name is needed for clarity here so normally we would
leave it out.

I am going to say r=me but I do have serious reservations. I'd like to hear
more concrete evidence about why this is a helpful change to make.