Introduce WebCore::DataView, a safer replacement for IPC::DataReference
Created attachment 349431 [details] Patch
Created attachment 349434 [details] Patch
Created attachment 349440 [details] Patch
Created attachment 349445 [details] Patch
Created attachment 349447 [details] Patch
Created attachment 349453 [details] Patch
Created attachment 349463 [details] Patch
Comment on attachment 349463 [details] Patch As far as I understand it (please correct me if I am wrong), there is no cost to calling data() / size() on the recipient side. On the recipient side, there is always only 1 segment. Therefore, requiring recipients to iterate over segments even though there is only one seems wrong and confusing. Recipients that need a buffer may actually iterate unnecessarily over the dataView. My understanding that is it would only be bad to call data() / size() on sender side, but this is not something that people do.
Added a few people knowledgeable with our IPC for opinions.
I think one alternative would be to tweak our IPC code generator to always pass a DataReference to the recipient, even if the .in uses SharedDataReference.
(In reply to Chris Dumez from comment #10) > I think one alternative would be to tweak our IPC code generator to always > pass a DataReference to the recipient, even if the .in uses > SharedDataReference. * SharedBufferDataReference
Comment on attachment 349463 [details] Patch *.messages.in always uses DataReferences right now. We create a SharedBufferDataReference as that DataReference right now where we know we are doing nothing but encoding it and don't want to copy the data into one segment. This leaves us to manually have to know we can't call .data() on some DataReferences but not others. This is the problem this patch is intended to fix. "I want to just assume all my data is in one segment" is also the reason we have SharedBuffer::data, which this will also help get rid of. That's another source of inefficient code. Re-setting r? hoping Chris will reconsider.
(In reply to Alex Christensen from comment #12) > Comment on attachment 349463 [details] > Patch > > *.messages.in always uses DataReferences right now. We create a > SharedBufferDataReference as that DataReference right now where we know we > are doing nothing but encoding it and don't want to copy the data into one > segment. This leaves us to manually have to know we can't call .data() on > some DataReferences but not others. This is the problem this patch is > intended to fix. > "I want to just assume all my data is in one segment" is also the reason we > have SharedBuffer::data, which this will also help get rid of. That's > another source of inefficient code. > Re-setting r? hoping Chris will reconsider. One the recipient side, there is always one segment, right? If so, I disagree that forcing the recipient to iterate over segments is a good idea.
(In reply to Chris Dumez from comment #13) > (In reply to Alex Christensen from comment #12) > > Comment on attachment 349463 [details] > > Patch > > > > *.messages.in always uses DataReferences right now. We create a > > SharedBufferDataReference as that DataReference right now where we know we > > are doing nothing but encoding it and don't want to copy the data into one > > segment. This leaves us to manually have to know we can't call .data() on > > some DataReferences but not others. This is the problem this patch is > > intended to fix. > > "I want to just assume all my data is in one segment" is also the reason we > > have SharedBuffer::data, which this will also help get rid of. That's > > another source of inefficient code. > > Re-setting r? hoping Chris will reconsider. > > One the recipient side, there is always one segment, right? If so, I > disagree that forcing the recipient to iterate over segments is a good idea. As in, if the recipient needs a uint8_t* / length, it will now likely have to construct a Vector and iterate over the dataView, appending to the vector. All this work / extra code would be unnecessary.
Comment on attachment 349463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349463&action=review > Source/WebCore/ChangeLog:9 > + This will help us write better code. I would argue this forces recipients to write less-efficient code if they need a uint64_t* / length because they'd now have to construct a vector and iterate over the DataView, appending each segment, even though there is always 1. SharedBufferDataReference is merely an encoding optimization at call sites, and the data() / size() method are already deleted on that class to discourage the sender from accessing the data (which is an unlikely use-case anyway).
Created attachment 349502 [details] Patch
Could someone look into why this latest patch doesn't work on Linux?
Comment on attachment 349502 [details] Patch Attachment 349502 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9187032 Number of test failures exceeded the failure limit.
Created attachment 349523 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 349502 [details] Patch Attachment 349502 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9187617 New failing tests: fast/text/variations/ipc2.html
Created attachment 349526 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Looks like something is trying to copy an IPC::Attachment, but it is noncopyable: In file included from ../../Source/WebKit/Platform/IPC/Decoder.h:32:0, from ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:28, from ../../Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.h:28, from ../../Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:29, from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/NetworkConnectionToWebProcessMessageReceiver.cpp:27: DerivedSources/ForwardingHeaders/wtf/Vector.h: In instantiation of ‘static void WTF::VectorCopier<false, T>::uninitializedCopy(const T*, const T*, U*) [with U = IPC::Attachment; T = IPC::Attachment]’: DerivedSources/ForwardingHeaders/wtf/Vector.h:265:79: required from ‘static void WTF::VectorTypeOperations<T>::uninitializedCopy(const T*, const T*, T*) [with T = IPC::Attachment]’ DerivedSources/ForwardingHeaders/wtf/Vector.h:885:42: required from ‘WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous> >::Vector(const WTF::Vector< <template-parameter-1-1>, <anonymous>, <template-parameter-1-3>, <anonymous> >&) [with T = IPC::Attachment; long unsigned int inlineCapacity = 0ul; OverflowHandler = WTF::CrashOnOverflow; long unsigned int minCapacity = 16ul]’ ../../Source/WebKit/Platform/IPC/Encoder.h:38:7: required from here DerivedSources/ForwardingHeaders/wtf/Vector.h:162:13: error: use of deleted function ‘constexpr IPC::Attachment::Attachment(const IPC::Attachment&)’ new (NotNull, dst) U(*src); ^~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from ../../Source/WebKit/Platform/IPC/Decoder.h:29:0, from ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:28, from ../../Source/WebKit/NetworkProcess/cache/CacheStorageEngineConnection.h:28, from ../../Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:29, from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/NetworkConnectionToWebProcessMessageReceiver.cpp:27: ../../Source/WebKit/Platform/IPC/Attachment.h:43:7: note: ‘constexpr IPC::Attachment::Attachment(const IPC::Attachment&)’ is implicitly declared as deleted because ‘IPC::Attachment’ declares a move constructor or move assignment operator class Attachment { ^~~~~~~~~~
Comment on attachment 349502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349502&action=review > Source/WebKit/Platform/IPC/DataReference.h:77 > virtual ~DataReference() { } Does this still need to be virtual? > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39 > + SharedBufferDataReference(const uint8_t* data, size_t size) Do we really need this constructor? Or are you only keeping it temporarily to help port the existing code incrementally? > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:60 > + Variant<std::pair<const uint8_t*, size_t>, Ref<const WebCore::SharedBuffer>> m_data; Ditto. Why cannot it be simply a SharedBuffer?
Comment on attachment 349502 [details] Patch We just have to figure out the GTK build failure and the layout test failures but it looks very nice otherwise.
Comment on attachment 349502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349502&action=review > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:45 > + [encoder] (const Ref<const WebCore::SharedBuffer>& buffer) mutable { I suspect &encoder might fix the build issues. We should not copy the encoder. > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:51 > + }, [encoder] (const std::pair<const uint8_t*, size_t>& pair) mutable { Ditto.
(In reply to Chris Dumez from comment #23) > Comment on attachment 349502 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349502&action=review > > > Source/WebKit/Platform/IPC/DataReference.h:77 > > virtual ~DataReference() { } > > Does this still need to be virtual? Probably not. > > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39 > > + SharedBufferDataReference(const uint8_t* data, size_t size) > > Do we really need this constructor? Or are you only keeping it temporarily > to help port the existing code incrementally? > > > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:60 > > + Variant<std::pair<const uint8_t*, size_t>, Ref<const WebCore::SharedBuffer>> m_data; > > Ditto. Why cannot it be simply a SharedBuffer? Messages::StorageProcess::DidReceiveFetchData is called once from a location that has a SharedBuffer and once from a location that has a pointer/size pair. I think this approach is more versatile than splitting that into two different calls that do the same thing with different parameter types.
Created attachment 349555 [details] Patch
Created attachment 349558 [details] Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Created attachment 349560 [details] Patch
Comment on attachment 349560 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349560&action=review r=me > Source/WebKit/Platform/IPC/SharedBufferDataReference.h:39 > + SharedBufferDataReference(const uint8_t* data, size_t size) I personally preference would have been to not have this constructor and use 2 separate IPC messages for the specific case that sometimes sends a SharedBuffer, somethings data+size. It looks confusing to me to construct a SharedBufferDataReference from anything else than a SharedBuffer but your call..
Comment on attachment 349560 [details] Patch Clearing flags on attachment: 349560 Committed r235951: <https://trac.webkit.org/changeset/235951>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44394559>