[Webkit-unassigned] [Bug 33222] Make text decoders and resource loaders read from segmented SharedBuffer

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 6 09:47:08 PST 2010


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
  Attachment #45965|review?                     |review+
               Flag|                            |

--- Comment #11 from Darin Adler <darin at apple.com>  2010-01-06 09:47:08 PST ---
(From update of attachment 45965)
> +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

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

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.

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list