Bug 45909 – BlobData should be copied for it to be used cross-thread in ThreadableBlobRegistry

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Jian Li]

BlobData should be copied for it to be used cross-thread in ThreadableBlobRegistry. This is to pull part of the fix from bug 45576. Other part of the fix remain in that bug and will be relanded when the discussion is settled.

[Jian Li]

Created attachment 67820 [details]
Proposed Patch

[David Levin]

Comment on attachment 67820 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=67820&action=prettypatch

> LayoutTests/ChangeLog:8
> +        Also fix a test issue. 

This is a bit generic. It would be nice to say what test issue is being fixed.

[Michael Nordman]

ownership of the BlobData instance is being passed to the registry... why does it need to be copied?

[Jian Li]

This is because BlobData contains String and KURL that need to be copied explicitly in order for it to be used in other thread.

[Michael Nordman]

what about the underlying CString's in BlobDataItems... do copies of those strings need to be made?

[Jian Li]

(In reply to comment #5)
> what about the underlying CString's in BlobDataItems... do copies of those strings need to be made?

This is fine. See comment at line 42 in BlobData.cpp.

[Michael Nordman]

I see that line...
    data = item.data; // This is OK because the underlying storage is Vector<char>.
... and it appears that a copy is being made

My point is do we need to make a copy since the registry has been given of ownership. It looks like a copy will be made and the original promptly deleted, so why make the copy?

Sorry for harping on the copies, but I think we should expect largish amounts of data in these elements. I can't help but notice when I see a copy being made. I haven't counted the total number of memcpys involved to actually get one of these registered in chrome... that should be interesting.

[David Levin]

(In reply to comment #7)
> I see that line...
>     data = item.data; // This is OK because the underlying storage is Vector<char>.
> ... and it appears that a copy is being made
> 
> My point is do we need to make a copy since the registry has been given of ownership. It looks like a copy will be made and the original promptly deleted, so why make the copy?
> 
> Sorry for harping on the copies, but I think we should expect largish amounts of data in these elements. I can't help but notice when I see a copy being made. I haven't counted the total number of memcpys involved to actually get one of these registered in chrome... that should be interesting.

fwiw, strings may be "copied" very quickly actually.

The key problem is that once you have a String (which is ref counted in a non-threadsafe manner) you don't know who else on the thread may have it also referenced.

It seems best that we get this code correct and then optimize.

[David Levin]

(In reply to comment #8)
> It seems best that we get this code correct and then optimize.

I meant to say optimize as needed. :)

[Michael Nordman]

Ok... I'll try to continue to point out the unneeded copies and continue to wonder how many years it will take for them to be removed ;)

[David Levin]

michaeln pointed out that this line is incorrrect: 
  data = item.data; // This is OK because the underlying storage is Vector<char>.

Because data isn't a Vector<char> nor a wrapper around one. It is a CString which is a thin wrapper around a RefCounted object.

[Jian Li]

(In reply to comment #11)
> michaeln pointed out that this line is incorrrect: 
>   data = item.data; // This is OK because the underlying storage is Vector<char>.
> 
> Because data isn't a Vector<char> nor a wrapper around one. It is a CString which is a thin wrapper around a RefCounted object.

Will fix. Thanks.

[Michael Nordman]

(In reply to comment #7)
> I see that line...
>     data = item.data; // This is OK because the underlying storage is Vector<char>.
> ... and it appears that a copy is being made
> 
> My point is do we need to make a copy since the registry has been given of ownership. It looks like a copy will be made and the original promptly deleted, so why make the copy?
> 
> Sorry for harping on the copies, but I think we should expect largish amounts of data in these elements. I can't help but notice when I see a copy being made. I haven't counted the total number of memcpys involved to actually get one of these registered in chrome... that should be interesting.

Some observations from our chat...

We're using 'copy()' semantics here in order to pass ownership from one thread to another. Copy works for that, but is also overkill for that since we don't need to retain an instance for use on the original thread. A method like 'detachFromThread' or 'disconnectFromThread' might make a better fit than this usage of blobData.copy().

I wonder if we should even have a copy() method on BlobData? I would prefer a way to produce a deep copy were not so readily available to help avoid that from happening.

COM has semantics like prepareToSwitchThreads() and switchThreads() for objects with thread affinity, where the former happens on the originating thread and the later on the new 'owning' thread. Maybe something along those lines for BlobData objects?

[Eric Seidel (no email)]

What's the status on this patch?

[Jian Li]

Forgot to close.

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

[David Levin]

(In reply to comment #15)
> Forgot to close.
> 
> Committed as http://trac.webkit.org/changeset/67646.

Is there a new bug for the semantics that Michael described to reduce copies?