45909 2010-09-16 11:07:57 -0700 BlobData should be copied for it to be used cross-thread in ThreadableBlobRegistry 2010-12-15 11:47:15 -0800 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jianli jianli eric levin michaeln oldest_to_newest 280217 0 jianli 2010-09-16 11:07:57 -0700 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. 280222 1 67820 jianli 2010-09-16 11:24:30 -0700 Created attachment 67820 Proposed Patch 280225 2 67820 levin 2010-09-16 11:27:50 -0700 Comment on attachment 67820 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. 280862 3 michaeln 2010-09-17 14:24:46 -0700 ownership of the BlobData instance is being passed to the registry... why does it need to be copied? 280868 4 jianli 2010-09-17 14:29:08 -0700 This is because BlobData contains String and KURL that need to be copied explicitly in order for it to be used in other thread. 280877 5 michaeln 2010-09-17 14:35:50 -0700 what about the underlying CString's in BlobDataItems... do copies of those strings need to be made? 280894 6 jianli 2010-09-17 15:01:30 -0700 (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. 280900 7 michaeln 2010-09-17 15:12:02 -0700 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. 280909 8 levin 2010-09-17 15:23:43 -0700 (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. 280910 9 levin 2010-09-17 15:24:39 -0700 (In reply to comment #8) > It seems best that we get this code correct and then optimize. I meant to say optimize as needed. :) 280933 10 michaeln 2010-09-17 15:37:30 -0700 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 ;) 280956 11 levin 2010-09-17 16:03:14 -0700 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. 280958 12 jianli 2010-09-17 16:06:19 -0700 (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. 280996 13 michaeln 2010-09-17 17:00:44 -0700 (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? 321840 14 eric 2010-12-14 01:59:24 -0800 What's the status on this patch? 322718 15 jianli 2010-12-15 11:42:03 -0800 Forgot to close. Committed as http://trac.webkit.org/changeset/67646. 322723 16 levin 2010-12-15 11:47:15 -0800 (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? 67820 2010-09-16 11:24:30 -0700 2010-09-16 11:27:49 -0700 Proposed Patch 45909 text/plain 2420 jianli ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCA1MDBhMDg2Li5iOTVjYWMxIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTAtMDkt MTYgIEppYW4gTGkgIDxqaWFubGlAY2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEJsb2JEYXRhIHNob3VsZCBiZSBjb3BpZWQgZm9y IGl0IHRvIGJlIHVzZWQgY3Jvc3MtdGhyZWFkIGluIFRocmVhZGFibGVCbG9iUmVnaXN0cnkuCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NTkwOQorCisg ICAgICAgIEFsc28gZml4IGEgdGVzdCBpc3N1ZS4gCisKKyAgICAgICAgKiBmYXN0L2ZpbGVzL2Fw cGx5LWJsb2ItdXJsLXRvLWltZy5odG1sOgorCiAyMDEwLTA5LTE2ICBaaGVueWFvIE1vICA8em1v QGdvb2dsZS5jb20+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgdGVzdCBUQUcgZml4LgpkaWZmIC0t Z2l0IGEvTGF5b3V0VGVzdHMvZmFzdC9maWxlcy9hcHBseS1ibG9iLXVybC10by1pbWcuaHRtbCBi L0xheW91dFRlc3RzL2Zhc3QvZmlsZXMvYXBwbHktYmxvYi11cmwtdG8taW1nLmh0bWwKaW5kZXgg ZTgxYTk1Zi4uZTFlZGFlZSAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvZmFzdC9maWxlcy9hcHBs eS1ibG9iLXVybC10by1pbWcuaHRtbAorKysgYi9MYXlvdXRUZXN0cy9mYXN0L2ZpbGVzL2FwcGx5 LWJsb2ItdXJsLXRvLWltZy5odG1sCkBAIC0yOSwxMCArMjksMTcgQEAgZnVuY3Rpb24gb25JbWdM b2FkKCkKICAgICAgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIubm90aWZ5RG9uZSgpOwogfQogCitm dW5jdGlvbiBtb3ZlTW91c2VUb0NlbnRlck9mRWxlbWVudChlbGVtZW50KQoreworICAgIHZhciBj ZW50ZXJYID0gZWxlbWVudC5vZmZzZXRMZWZ0ICsgZWxlbWVudC5vZmZzZXRXaWR0aCAvIDI7Cisg ICAgdmFyIGNlbnRlclkgPSBlbGVtZW50Lm9mZnNldFRvcCArIGVsZW1lbnQub2Zmc2V0SGVpZ2h0 IC8gMjsKKyAgICBldmVudFNlbmRlci5tb3VzZU1vdmVUbyhjZW50ZXJYLCBjZW50ZXJZKTsKK30K KwogZnVuY3Rpb24gcnVuVGVzdHMoKQogewogICAgIGV2ZW50U2VuZGVyLmJlZ2luRHJhZ1dpdGhG aWxlcyhbJ3Jlc291cmNlcy9hYmUucG5nJ10pOwotICAgIGV2ZW50U2VuZGVyLm1vdXNlTW92ZVRv KDEwLCAxMCk7CisgICAgbW92ZU1vdXNlVG9DZW50ZXJPZkVsZW1lbnQoZG9jdW1lbnQuZ2V0RWxl bWVudEJ5SWQoJ2ZpbGUnKSk7CiAgICAgZXZlbnRTZW5kZXIubW91c2VVcCgpOwogfQogCmRpZmYg LS1naXQgYS9XZWJDb3JlL0NoYW5nZUxvZyBiL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IGQyMGZi YTAuLmE0MzA2NzUgMTAwNjQ0Ci0tLSBhL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1dlYkNvcmUv Q2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTAtMDktMTYgIEppYW4gTGkgIDxqaWFubGlA Y2hyb21pdW0ub3JnPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgIEJsb2JEYXRhIHNob3VsZCBiZSBjb3BpZWQgZm9yIGl0IHRvIGJlIHVzZWQgY3Jvc3Mt dGhyZWFkIGluIFRocmVhZGFibGVCbG9iUmVnaXN0cnkuCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NTkwOQorCisgICAgICAgICogZmlsZWFwaS9UaHJl YWRhYmxlQmxvYlJlZ2lzdHJ5LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkJsb2JSZWdpc3RyeUNv bnRleHQ6OkJsb2JSZWdpc3RyeUNvbnRleHQpOgorCiAyMDEwLTA5LTE2ICBEYW5pZWwgQ2hlbmcg IDxkY2hlbmdAY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IFRvbnkgQ2hhbmcu CmRpZmYgLS1naXQgYS9XZWJDb3JlL2ZpbGVhcGkvVGhyZWFkYWJsZUJsb2JSZWdpc3RyeS5jcHAg Yi9XZWJDb3JlL2ZpbGVhcGkvVGhyZWFkYWJsZUJsb2JSZWdpc3RyeS5jcHAKaW5kZXggN2UwNzQ1 Zi4uZjc0YzY4MCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9maWxlYXBpL1RocmVhZGFibGVCbG9iUmVn aXN0cnkuY3BwCisrKyBiL1dlYkNvcmUvZmlsZWFwaS9UaHJlYWRhYmxlQmxvYlJlZ2lzdHJ5LmNw cApAQCAtNDEsNyArNDEsNyBAQCBuYW1lc3BhY2UgV2ViQ29yZSB7CiBzdHJ1Y3QgQmxvYlJlZ2lz dHJ5Q29udGV4dCB7CiAgICAgQmxvYlJlZ2lzdHJ5Q29udGV4dChjb25zdCBLVVJMJiB1cmwsIFBh c3NPd25QdHI8QmxvYkRhdGE+IGJsb2JEYXRhKQogICAgICAgICA6IHVybCh1cmwuY29weSgpKQot ICAgICAgICAsIGJsb2JEYXRhKGJsb2JEYXRhKQorICAgICAgICAsIGJsb2JEYXRhKGJsb2JEYXRh LT5jb3B5KCkpCiAgICAgewogICAgIH0KIAo=