Bug 44884 – Improve BlobBuilder to combine adjacent strings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

Improve BlobBuilder to combine adjacent strings.

[Jian Li]

Created attachment 65936 [details]
Proposed Patch

[Kinuko Yasuda]

(In reply to comment #1)
> Created an attachment (id=65936) [details]
> Proposed Patch

I wonder if this may end up copying strings too many times.
What if the user script calls blobBuilder.append(string) hundreds of times?
Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()?

[Jian Li]

Committed as http://trac.webkit.org/changeset/66499.

[Jian Li]

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=65936) [details] [details]
> > Proposed Patch
> 
> I wonder if this may end up copying strings too many times.
> What if the user script calls blobBuilder.append(string) hundreds of times?
> Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()?

I think it is fine since this is what we also do for combining string in form data.

[Kinuko Yasuda]

(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > Created an attachment (id=65936) [details] [details] [details]
> > > Proposed Patch
> > 
> > I wonder if this may end up copying strings too many times.
> > What if the user script calls blobBuilder.append(string) hundreds of times?
> > Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()?
> 
> I think it is fine since this is what we also do for combining string in form data.

Do you mean FormData::appendData()?
It calls Vector.grow() which utilizes memory (slightly) more efficiently and does not copy the original data if the new size fits in the reserved capacity.  (I was wondering why we don't simply use Vector.append() though.)
In this patch every time we call append we copy both original and new strings.
It may make sense in a bigger redesign but I don't quite get it why we needed this particular small change separately.

[Michael Nordman]

> > > I wonder if this may end up copying strings too many times.
> > > What if the user script calls blobBuilder.append(string) hundreds of times?
> > > Can't we put a FIXME to optimize the performance later or isn't it better to concatenate the strings at once on getBlob()?
> > 
> > I think it is fine since this is what we also do for combining string in form data.
> 
> Do you mean FormData::appendData()?
> It calls Vector.grow() which utilizes memory (slightly) more efficiently and does not copy the original data if the new size fits in the reserved capacity.  (I was wondering why we don't simply use Vector.append() though.)
> In this patch every time we call append we copy both original and new strings.
> It may make sense in a bigger redesign but I don't quite get it why we needed this particular small change separately.

I agree with kinuko about this particular patch not really helping the matter. Maybe we should leave the code as is until something more thoughtful is put together.