Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2058633002:
mojo::ArrayTraits: Add Support for Iterators (Closed)
|
Created:
4 years, 6 months ago by Fady Samuel Modified:
4 years, 6 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org, Ken Rockot(use gerrit already) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmojo::ArrayTraits: Add Support for Iterators
This CL adds support for iterating over a data structure to serialize over
mojo as an array wire format.
This CL also modifies changes to LatencyInfo StructTraits that avoids
extra copies of LatencyMap on serialization.
Note that this solution doesn't fully address deserialization though.
You cannot directly assign an std::pair to a map iterator, for example.
The solution is to introduce an ArrayTraits::Assign. I'll do that in a
separate CL as this CL has already gotten very complex.
BUG=611802
TBR=ben@chromium.org
Committed: https://crrev.com/09ab6968c2bb80dc2ea64a56fbd4a60b632af4ca
Cr-Commit-Position: refs/heads/master@{#399479}
Patch Set 1 #Patch Set 2 : Simpification #Patch Set 3 : Improved LatencyInfo! #Patch Set 4 : Rebased #
Total comments: 8
Patch Set 5 : Addressed comments #
Total comments: 5
Patch Set 6 : Addressed Yuzhu's comment #Patch Set 7 : Minor simplification #
Total comments: 2
Patch Set 8 : Addressed nit #
Messages
Total messages: 42 (16 generated)
Description was changed from ========== [WIP] ArrayTraits: Add Support for Iterators BUG= ========== to ========== mojo::ArrayTraits: Add Support for Iterators This CL adds support for iterating over a data structure to serialize over mojo as an array wire format. This CL also modifies changes to LatencyInfo StructTraits that avoids extra copies of LatencyMap on serialization. Note that this solution doesn't fully address deserialization though. You cannot directly assign an std::pair to a map iterator, for example. The solution is to introduce an ArrayTraits::Assign. I'll do that in a separate CL as this CL has already gotten very complex. BUG=611802 ==========
fsamuel@chromium.org changed reviewers: + sky@chromium.org, tsepez@chromium.org, yzshen@chromium.org
+yzshen@ for review +sky@ once Yuzhu is happy for OWNER stamp +tsepez@ for ipc
+rockot@ to cc as FYI
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058633002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058633002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Am I correct in thinking that this doesn't change the on-the-wire format, instead producing a set of templated helpers that put an arbitrary array onto the wire in the same manner as one would manually have written?
On 2016/06/10 16:06:52, Tom Sepez wrote: > Am I correct in thinking that this doesn't change the on-the-wire format, > instead producing a set of templated helpers that put an arbitrary array onto > the wire in the same manner as one would manually have written? Yup, the goal here is to add some template helpers within mojo bindings so that containers can be serialized directly to the wire format (and deserialized back into the output container) that don't have O(1) random access.
fsamuel@chromium.org changed reviewers: + ben@chromium.org - sky@chromium.org
-sky@ who is OOO, +ben@ for OWNER stamp once Yuzhu is happy/
On 2016/06/10 16:17:57, Fady Samuel wrote: > On 2016/06/10 16:06:52, Tom Sepez wrote: > > Am I correct in thinking that this doesn't change the on-the-wire format, > > instead producing a set of templated helpers that put an arbitrary array onto > > the wire in the same manner as one would manually have written? > > Yup, the goal here is to add some template helpers within mojo bindings so that > containers can be serialized directly to the wire format (and deserialized back > into the output container) that don't have O(1) random access. Ok, LGTM, given the deserialization part comes next and will require more scrutiny. Thanks
https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:68: auto& value = Traits::GetValue(iter); Please update the comments in array_traits.h https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:87: class ArrayReader { Maybe we should change this to ArrayIterator and let DeserializeElements constructs an ArrayIterator after Resize()? That way there is no need to extract GetNextHelper as standalone functions. And the callsites seem more consistent too. https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/serialization_util.h (right): https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/serialization_util.h:243: struct HasGetDataMethod { nit: to be consistent with the surrounding code, please move this struct to be right above the CallGetDataIfExists() functions. https://codereview.chromium.org/2058633002/diff/60001/ui/events/mojo/latency_... File ui/events/mojo/latency_info_struct_traits.h (right): https://codereview.chromium.org/2058633002/diff/60001/ui/events/mojo/latency_... ui/events/mojo/latency_info_struct_traits.h:39: return true; Why this can be true without any action?
PTAL Yuzhu! https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:68: auto& value = Traits::GetValue(iter); On 2016/06/10 16:21:32, yzshen1 wrote: > Please update the comments in array_traits.h Done. https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:87: class ArrayReader { On 2016/06/10 16:21:32, yzshen1 wrote: > Maybe we should change this to ArrayIterator and let DeserializeElements > constructs an ArrayIterator after Resize()? > > That way there is no need to extract GetNextHelper as standalone functions. And > the callsites seem more consistent too. Done. I'm not sure how to get rid of GetNextHelper. I *think* we need it for SFINAE. Also, I tried making it a static private method and failed. I wasn't able to get it to compile, but my C++11 template knowledge is limited. https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/serialization_util.h (right): https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/serialization_util.h:243: struct HasGetDataMethod { On 2016/06/10 16:21:32, yzshen1 wrote: > nit: to be consistent with the surrounding code, please move this struct to be > right above the CallGetDataIfExists() functions. Done. https://codereview.chromium.org/2058633002/diff/60001/ui/events/mojo/latency_... File ui/events/mojo/latency_info_struct_traits.h (right): https://codereview.chromium.org/2058633002/diff/60001/ui/events/mojo/latency_... ui/events/mojo/latency_info_struct_traits.h:39: return true; On 2016/06/10 16:21:32, yzshen1 wrote: > Why this can be true without any action? LatencyMap doesn't have a fixed size that has to be resized, rather it grows dynamically as elements are inserted. Thus "Reserve" might be a better word for this and we might rename this in the future, but I'll punt on it for now.
PTAL Yuzhu! https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:68: auto& value = Traits::GetValue(iter); On 2016/06/10 16:21:32, yzshen1 wrote: > Please update the comments in array_traits.h Done. https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:87: class ArrayReader { On 2016/06/10 16:21:32, yzshen1 wrote: > Maybe we should change this to ArrayIterator and let DeserializeElements > constructs an ArrayIterator after Resize()? > > That way there is no need to extract GetNextHelper as standalone functions. And > the callsites seem more consistent too. Done. I'm not sure how to get rid of GetNextHelper. I *think* we need it for SFINAE. Also, I tried making it a static private method and failed. I wasn't able to get it to compile, but my C++11 template knowledge is limited. https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/serialization_util.h (right): https://codereview.chromium.org/2058633002/diff/60001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/serialization_util.h:243: struct HasGetDataMethod { On 2016/06/10 16:21:32, yzshen1 wrote: > nit: to be consistent with the surrounding code, please move this struct to be > right above the CallGetDataIfExists() functions. Done. https://codereview.chromium.org/2058633002/diff/60001/ui/events/mojo/latency_... File ui/events/mojo/latency_info_struct_traits.h (right): https://codereview.chromium.org/2058633002/diff/60001/ui/events/mojo/latency_... ui/events/mojo/latency_info_struct_traits.h:39: return true; On 2016/06/10 16:21:32, yzshen1 wrote: > Why this can be true without any action? LatencyMap doesn't have a fixed size that has to be resized, rather it grows dynamically as elements are inserted. Thus "Reserve" might be a better word for this and we might rename this in the future, but I'll punt on it for now.
https://codereview.chromium.org/2058633002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/2058633002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:65: static decltype(Traits::GetValue(std::declval<IteratorType&>())) GetNextHelper( Is it possible to merge these two helpers into ArrayIterator? Maybe the easiest way is to have two ArrayIterator specialization (one uses GetAt; the other iterator). I think the code will be easier to read than this. :) WDYT? https://codereview.chromium.org/2058633002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:141: using IteratorType = This is not needed anymore. (and same below) https://codereview.chromium.org/2058633002/diff/80001/ui/events/mojo/latency_... File ui/events/mojo/latency_info_struct_traits.h (right): https://codereview.chromium.org/2058633002/diff/80001/ui/events/mojo/latency_... ui/events/mojo/latency_info_struct_traits.h:38: static bool Resize(ui::LatencyInfo::LatencyMap& input, size_t size) { It seems this ArrayTraits is not used for deserialization, maybe you can omit Resize()? That way if we do need to use it for deserialization, we will know by compilation errors.
PTAL Yuzhu! I'm by no means a template expert (especially C++ 11 templates), so please let me know if there's a better way to do this! Thanks! https://codereview.chromium.org/2058633002/diff/80001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/2058633002/diff/80001/mojo/public/cpp/binding... mojo/public/cpp/bindings/lib/array_serialization.h:65: static decltype(Traits::GetValue(std::declval<IteratorType&>())) GetNextHelper( On 2016/06/11 00:40:27, yzshen1 wrote: > Is it possible to merge these two helpers into ArrayIterator? > Maybe the easiest way is to have two ArrayIterator specialization (one uses > GetAt; the other iterator). I think the code will be easier to read than this. > :) > > WDYT? I agree it's probably easier to understand although it's maybe a bit more code. Done. Note that since this is no longer method overloading I can't use SFINAE, so instead I have two specializations, one that HasGetBegin, and one that does not. I use HasGetBeginMethod to make that decision and I all that ArrayIterator. https://codereview.chromium.org/2058633002/diff/80001/ui/events/mojo/latency_... File ui/events/mojo/latency_info_struct_traits.h (right): https://codereview.chromium.org/2058633002/diff/80001/ui/events/mojo/latency_... ui/events/mojo/latency_info_struct_traits.h:38: static bool Resize(ui::LatencyInfo::LatencyMap& input, size_t size) { On 2016/06/11 00:40:27, yzshen1 wrote: > It seems this ArrayTraits is not used for deserialization, maybe you can omit > Resize()? > That way if we do need to use it for deserialization, we will know by > compilation errors. Done.
PTAL, a small simplification.
The CQ bit was checked by fsamuel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058633002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
LGTM with one nit. Thanks! https://codereview.chromium.org/2058633002/diff/120001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/2058633002/diff/120001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/array_serialization.h:146: using IteratorType = please remove this line.
Description was changed from ========== mojo::ArrayTraits: Add Support for Iterators This CL adds support for iterating over a data structure to serialize over mojo as an array wire format. This CL also modifies changes to LatencyInfo StructTraits that avoids extra copies of LatencyMap on serialization. Note that this solution doesn't fully address deserialization though. You cannot directly assign an std::pair to a map iterator, for example. The solution is to introduce an ArrayTraits::Assign. I'll do that in a separate CL as this CL has already gotten very complex. BUG=611802 ========== to ========== mojo::ArrayTraits: Add Support for Iterators This CL adds support for iterating over a data structure to serialize over mojo as an array wire format. This CL also modifies changes to LatencyInfo StructTraits that avoids extra copies of LatencyMap on serialization. Note that this solution doesn't fully address deserialization though. You cannot directly assign an std::pair to a map iterator, for example. The solution is to introduce an ArrayTraits::Assign. I'll do that in a separate CL as this CL has already gotten very complex. BUG=611802 TBR=ben@chromium.org ==========
CQ'ing! Thanks! https://codereview.chromium.org/2058633002/diff/120001/mojo/public/cpp/bindin... File mojo/public/cpp/bindings/lib/array_serialization.h (right): https://codereview.chromium.org/2058633002/diff/120001/mojo/public/cpp/bindin... mojo/public/cpp/bindings/lib/array_serialization.h:146: using IteratorType = On 2016/06/13 15:18:05, yzshen1 wrote: > please remove this line. Done.
The CQ bit was checked by fsamuel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2058633002/#ps140001 (title: "Addressed nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058633002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by fsamuel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2058633002/140001
Message was sent while issue was closed.
Description was changed from ========== mojo::ArrayTraits: Add Support for Iterators This CL adds support for iterating over a data structure to serialize over mojo as an array wire format. This CL also modifies changes to LatencyInfo StructTraits that avoids extra copies of LatencyMap on serialization. Note that this solution doesn't fully address deserialization though. You cannot directly assign an std::pair to a map iterator, for example. The solution is to introduce an ArrayTraits::Assign. I'll do that in a separate CL as this CL has already gotten very complex. BUG=611802 TBR=ben@chromium.org ========== to ========== mojo::ArrayTraits: Add Support for Iterators This CL adds support for iterating over a data structure to serialize over mojo as an array wire format. This CL also modifies changes to LatencyInfo StructTraits that avoids extra copies of LatencyMap on serialization. Note that this solution doesn't fully address deserialization though. You cannot directly assign an std::pair to a map iterator, for example. The solution is to introduce an ArrayTraits::Assign. I'll do that in a separate CL as this CL has already gotten very complex. BUG=611802 TBR=ben@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== mojo::ArrayTraits: Add Support for Iterators This CL adds support for iterating over a data structure to serialize over mojo as an array wire format. This CL also modifies changes to LatencyInfo StructTraits that avoids extra copies of LatencyMap on serialization. Note that this solution doesn't fully address deserialization though. You cannot directly assign an std::pair to a map iterator, for example. The solution is to introduce an ArrayTraits::Assign. I'll do that in a separate CL as this CL has already gotten very complex. BUG=611802 TBR=ben@chromium.org ========== to ========== mojo::ArrayTraits: Add Support for Iterators This CL adds support for iterating over a data structure to serialize over mojo as an array wire format. This CL also modifies changes to LatencyInfo StructTraits that avoids extra copies of LatencyMap on serialization. Note that this solution doesn't fully address deserialization though. You cannot directly assign an std::pair to a map iterator, for example. The solution is to introduce an ArrayTraits::Assign. I'll do that in a separate CL as this CL has already gotten very complex. BUG=611802 TBR=ben@chromium.org Committed: https://crrev.com/09ab6968c2bb80dc2ea64a56fbd4a60b632af4ca Cr-Commit-Position: refs/heads/master@{#399479} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/09ab6968c2bb80dc2ea64a56fbd4a60b632af4ca Cr-Commit-Position: refs/heads/master@{#399479} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
