Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2522633004:
Add LazySyncableAdapter (Closed)
|
Created:
4 years, 1 month ago by steimel Modified:
4 years ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, bgoldman+watch-blimp_chromium.org, steimel+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, perumaal+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, scf+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd LazySyncableAdapter
The LazySyncableAdapter is used to wrap LazySyncables of any changeset type into a LazySyncable with a string changeset by serializing the changesets into a string and parsing them from strings. That way, ObjectSyncState can just work with LazySyncables of type string.
BUG=663065
Committed: https://crrev.com/cfafbc577a940d5891487741aa0408497749fb57
Cr-Commit-Position: refs/heads/master@{#436137}
Patch Set 1 #
Total comments: 11
Patch Set 2 : scf feedback #
Total comments: 10
Patch Set 3 : kmarshall feedback #
Total comments: 4
Patch Set 4 : minor cleanup #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Add LazySyncableAdapter BUG=663065 ========== to ========== Add LazySyncableAdapter The LazySyncableAdapter is used to wrap LazySyncables of any changeset type into a LazySyncable with a string changeset by serializing the changesets into a string and parsing them from strings. That way, ObjectSyncState can just work with LazySyncables of type string. BUG=663065 ==========
steimel@chromium.org changed reviewers: + kmarshall@chromium.org, lethalantidote@chromium.org, scf@chromium.org
PTAL
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... File blimp/helium/lazy_syncable_adapter.h (right): https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:41: explicit LazySyncableAdapter(LazySyncable<ChangesetType>* object) nit: rename to inner_syncable_ ? https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:50: const base::Closure& local_update_callback) override { for consistency put implementation outside? https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:53: void ReleaseBefore(Revision checkpoint) override { according to @wez i think this was renamed to |before| https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:70: std::unique_ptr<ChangesetType> changeset = object_->CreateChangeset(from); It feels to me that there is a bit of inefficiency here. I assume 99% of the objects being synchronized will be CompoundSyncables which have an interface CreateChangeset(Revision from, google::protobuf::io::CodedOutputStream* output_stream) which would be perfect use for the ObjectSyncState. Many CodedOutputStreams and strings are being created with the current approach. Chatted offline with @wez, he suggested one thing we could do is have something similar to ``` CreateChangeset( Revision from, google::protobuf::io::CodedOutputStream* output_stream) ``` At a new class which is the base of Syncable. This class is not templatized so that ObjectSyncState can use directly. ObjectSyncState could use this and this could simplify the implementation of CompoundSyncable too. Something like the following: * BaseLazeSyncable: Not templatized class that contains all methods of LazySyncables with correspondent Create and Apply changeset that would receive a CodedOutputStream. * BaseChangesetLazySyncable: Provides default implementation for serializing changesets in coded output streams. Basically calls the Serializable methods (Serialize/Parse) * CompoundSyncable: Gets updated to use this interface. WDYT?
steimel@chromium.org changed reviewers: + wez@chromium.org
PTAL https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... File blimp/helium/lazy_syncable_adapter.h (right): https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:41: explicit LazySyncableAdapter(LazySyncable<ChangesetType>* object) On 2016/11/22 23:35:23, scf wrote: > nit: rename to inner_syncable_ ? Done. https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:50: const base::Closure& local_update_callback) override { On 2016/11/22 23:35:22, scf wrote: > for consistency put implementation outside? Done. https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:53: void ReleaseBefore(Revision checkpoint) override { On 2016/11/22 23:35:23, scf wrote: > according to @wez i think this was renamed to |before| Okay, changed. Strangely enough, it's currently |checkpoint| in syncable.h, but the comment above it uses |before|. Something should be changed there https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:70: std::unique_ptr<ChangesetType> changeset = object_->CreateChangeset(from); On 2016/11/22 23:35:22, scf wrote: > It feels to me that there is a bit of inefficiency here. I assume 99% of the > objects being synchronized will be CompoundSyncables which have an interface > CreateChangeset(Revision from, google::protobuf::io::CodedOutputStream* > output_stream) which would be perfect use for the ObjectSyncState. Many > CodedOutputStreams and strings are being created with the current approach. > > Chatted offline with @wez, he suggested one thing we could do is have something > similar to > > ``` > CreateChangeset( > Revision from, > google::protobuf::io::CodedOutputStream* output_stream) > ``` > > At a new class which is the base of Syncable. This class is not templatized so > that ObjectSyncState can use directly. ObjectSyncState could use this and this > could simplify the implementation of CompoundSyncable too. > > Something like the following: > * BaseLazeSyncable: Not templatized class that contains all methods of > LazySyncables with correspondent Create and Apply changeset that would receive a > CodedOutputStream. > * BaseChangesetLazySyncable: Provides default implementation for serializing > changesets in coded output streams. Basically calls the Serializable methods > (Serialize/Parse) > * CompoundSyncable: Gets updated to use this interface. > > > WDYT? > That's pretty much exactly what Kevin had in his original HybridSyncable design (Syncable was top-level non-templated with Coded* and HybridSyncable was templated and used Serialize/Parse). I actually liked that setup and would be fine moving back to it. Thoughts?
https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... File blimp/helium/lazy_syncable_adapter.h (right): https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:21: std::unique_ptr<ChangesetType> ParseChangesetFromString( Just curious, why is this function in an anon namespace?
https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... File blimp/helium/lazy_syncable_adapter.h (right): https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:21: std::unique_ptr<ChangesetType> ParseChangesetFromString( On 2016/11/23 22:39:27, CJ wrote: > Just curious, why is this function in an anon namespace? I was on the fence as to whether it should be a private function of the class or just a outside helper function, but since it doesn't need to know any members or anything, I decided to make it a helper function. Since it's a helper function that's only used within this file and I didn't want to pollute the blimp::helium namespace, I put it in an anon namespace
https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... File blimp/helium/lazy_syncable_adapter.h (right): https://codereview.chromium.org/2522633004/diff/1/blimp/helium/lazy_syncable_... blimp/helium/lazy_syncable_adapter.h:21: std::unique_ptr<ChangesetType> ParseChangesetFromString( On 2016/11/28 17:19:26, steimel wrote: > On 2016/11/23 22:39:27, CJ wrote: > > Just curious, why is this function in an anon namespace? > > I was on the fence as to whether it should be a private function of the class or > just a outside helper function, but since it doesn't need to know any members or > anything, I decided to make it a helper function. Since it's a helper function > that's only used within this file and I didn't want to pollute the blimp::helium > namespace, I put it in an anon namespace Acknowledged.
https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... File blimp/helium/lazy_syncable_adapter.h (right): https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter.h:21: std::unique_ptr<ChangesetType> ParseChangesetFromString( Anonymous namespaced functions in the .h aren't actually hidden from anything. They only really make sense in the .cc. A static class private method makes more sense here. https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... File blimp/helium/lazy_syncable_adapter_unittest.cc (right): https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:68: base::Closure callback_; Nit: it seems unnecessary to bind a class member for something so trivial - why not just Bind(DoNothing) in the calls directly? https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:69: Revision revision_ = 42; Make this a constexpr at the top instead of a class member. https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:76: EXPECT_CALL(syncable_, SetLocalUpdateCallback(_)).Times(1); nit: Times(1) is superfluous; remove it? https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:109: .WillOnce(DoAll(SaveArg<0>(&validate_changeset), Return(true))); Instead of saving the arg, try this in the matcher expectations: Field(&TestSyncableChangeset::value, Equals(33)) This is possible because value supports operator().
PTAL https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... File blimp/helium/lazy_syncable_adapter.h (right): https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter.h:21: std::unique_ptr<ChangesetType> ParseChangesetFromString( On 2016/12/02 18:28:03, Kevin M wrote: > Anonymous namespaced functions in the .h aren't actually hidden from anything. > They only really make sense in the .cc. > > A static class private method makes more sense here. Good point. Done. https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... File blimp/helium/lazy_syncable_adapter_unittest.cc (right): https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:68: base::Closure callback_; On 2016/12/02 18:28:03, Kevin M wrote: > Nit: it seems unnecessary to bind a class member for something so trivial - why > not just Bind(DoNothing) in the calls directly? Done. Slight change in how those are working since now I'm checking that it's sending the correct callback https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:69: Revision revision_ = 42; On 2016/12/02 18:28:03, Kevin M wrote: > Make this a constexpr at the top instead of a class member. Done. https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:76: EXPECT_CALL(syncable_, SetLocalUpdateCallback(_)).Times(1); On 2016/12/02 18:28:03, Kevin M wrote: > nit: Times(1) is superfluous; remove it? Done. https://codereview.chromium.org/2522633004/diff/20001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:109: .WillOnce(DoAll(SaveArg<0>(&validate_changeset), Return(true))); On 2016/12/02 18:28:03, Kevin M wrote: > Instead of saving the arg, try this in the matcher expectations: > > Field(&TestSyncableChangeset::value, Equals(33)) > > This is possible because value supports operator(). Discussed offline and sticking with SaveArg
lgtm https://codereview.chromium.org/2522633004/diff/40001/blimp/helium/lazy_synca... File blimp/helium/lazy_syncable_adapter_unittest.cc (right): https://codereview.chromium.org/2522633004/diff/40001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:10: #include "base/bind_helpers.h" Not used? https://codereview.chromium.org/2522633004/diff/40001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:25: constexpr Revision revision = 42; kRevision
https://codereview.chromium.org/2522633004/diff/40001/blimp/helium/lazy_synca... File blimp/helium/lazy_syncable_adapter_unittest.cc (right): https://codereview.chromium.org/2522633004/diff/40001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:10: #include "base/bind_helpers.h" On 2016/12/03 00:35:01, Kevin M wrote: > Not used? Done. https://codereview.chromium.org/2522633004/diff/40001/blimp/helium/lazy_synca... blimp/helium/lazy_syncable_adapter_unittest.cc:25: constexpr Revision revision = 42; On 2016/12/03 00:35:01, Kevin M wrote: > kRevision Done.
The CQ bit was checked by steimel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kmarshall@chromium.org Link to the patchset: https://codereview.chromium.org/2522633004/#ps60001 (title: "minor cleanup")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480725676471340,
"parent_rev": "2aa013d229401436cf553dd68f98d21d4be0a53d", "commit_rev":
"6ea6992fb38747f57ed69fa78fc1e6cfcd35cc64"}
Message was sent while issue was closed.
Description was changed from ========== Add LazySyncableAdapter The LazySyncableAdapter is used to wrap LazySyncables of any changeset type into a LazySyncable with a string changeset by serializing the changesets into a string and parsing them from strings. That way, ObjectSyncState can just work with LazySyncables of type string. BUG=663065 ========== to ========== Add LazySyncableAdapter The LazySyncableAdapter is used to wrap LazySyncables of any changeset type into a LazySyncable with a string changeset by serializing the changesets into a string and parsing them from strings. That way, ObjectSyncState can just work with LazySyncables of type string. BUG=663065 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add LazySyncableAdapter The LazySyncableAdapter is used to wrap LazySyncables of any changeset type into a LazySyncable with a string changeset by serializing the changesets into a string and parsing them from strings. That way, ObjectSyncState can just work with LazySyncables of type string. BUG=663065 ========== to ========== Add LazySyncableAdapter The LazySyncableAdapter is used to wrap LazySyncables of any changeset type into a LazySyncable with a string changeset by serializing the changesets into a string and parsing them from strings. That way, ObjectSyncState can just work with LazySyncables of type string. BUG=663065 Committed: https://crrev.com/cfafbc577a940d5891487741aa0408497749fb57 Cr-Commit-Position: refs/heads/master@{#436137} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/cfafbc577a940d5891487741aa0408497749fb57 Cr-Commit-Position: refs/heads/master@{#436137} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
