Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2598113002:
[Sync] Use a proto to generate AutofillSyncStorageKey's storage keys. (Closed)
|
Created:
3 years, 12 months ago by skym Modified:
3 years, 11 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, Patrick Noland, maxbogue, Gang Wu Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Use a proto to generate AutofillSyncStorageKey's storage keys.
Previously we were going to use a | delimiter and manually construct
storage keys ourselves. The requires us to write logic to reverse it,
and is inefficient when encoding characters that need to be escaped.
The disadvantages are that we always have 4 bytes (instead of 1) of
overhead, and printing the string is less readable.
BUG=675992
Review-Url: https://codereview.chromium.org/2598113002
Cr-Commit-Position: refs/heads/master@{#442011}
Committed: https://chromium.googlesource.com/chromium/src/+/ba936951fafeeb20fe1ecb72e3ec40b98bce9fee
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebased #
Total comments: 11
Patch Set 3 : Rebased and updated for comments. #
Total comments: 6
Patch Set 4 : Updated for mathp's comments. #
Messages
Total messages: 35 (22 generated)
The CQ bit was checked by skym@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...
Description was changed from ========== [Sync] Use a proto to generate AutofillSyncStorageKey's storage keys. BUG=675992 ========== to ========== [Sync] Use a proto to generate AutofillSyncStorageKey's storage keys. BUG=675992 ==========
skym@chromium.org changed reviewers: + gangwu@chromium.org, maxbogue@chromium.org, pavely@chromium.org, pnoland@chromium.org
I thought we were planning on using a proto for storage keys, not a delimiter and manual parsing. The delimiter approach is probably less bytes in most cases. Proto seems to cause 4 bytes overhead on every storage key while delimiter is only 1 byte. But for any value that uses characters that need to be escaped, which includes everything non-ascii, we seem to bloat 3x with net::EscapePath. We're gonna hold these values in memory, forever. WDYT? There was also the talk of a short storage key optimization. If we wanted to go down this road, it might make the most sense to do it prior to launch as well. https://codereview.chromium.org/2598113002/diff/1/components/autofill/core/br... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h (right): https://codereview.chromium.org/2598113002/diff/1/components/autofill/core/br... components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:62: // Generate a string key uniquely identifying |entity_data| in the context of I missed updating this comment.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/23 00:34:09, skym wrote: > I thought we were planning on using a proto for storage keys, not a delimiter > and manual parsing. The delimiter approach is probably less bytes in most cases. > Proto seems to cause 4 bytes overhead on every storage key while delimiter is > only 1 byte. But for any value that uses characters that need to be escaped, > which includes everything non-ascii, we seem to bloat 3x with net::EscapePath. > We're gonna hold these values in memory, forever. WDYT? > > There was also the talk of a short storage key optimization. If we wanted to go > down this road, it might make the most sense to do it prior to launch as well. > > https://codereview.chromium.org/2598113002/diff/1/components/autofill/core/br... > File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h > (right): > > https://codereview.chromium.org/2598113002/diff/1/components/autofill/core/br... > components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:62: // > Generate a string key uniquely identifying |entity_data| in the context of > I missed updating this comment. From what I can tell, EscapePath (https://cs.chromium.org/chromium/src/net/base/escape.cc?dr=CSs&sq=package:chr...) only escapes characters in the kPathCharMap: "#%:<>?[\]^`{|} . I would be surprised if escaping these characters led to a lot of overhead in practice.
On 2017/01/03 20:08:16, Patrick Noland wrote: > On 2016/12/23 00:34:09, skym wrote: > > I thought we were planning on using a proto for storage keys, not a delimiter > > and manual parsing. The delimiter approach is probably less bytes in most > cases. > > Proto seems to cause 4 bytes overhead on every storage key while delimiter is > > only 1 byte. But for any value that uses characters that need to be escaped, > > which includes everything non-ascii, we seem to bloat 3x with net::EscapePath. > > We're gonna hold these values in memory, forever. WDYT? > > > > There was also the talk of a short storage key optimization. If we wanted to > go > > down this road, it might make the most sense to do it prior to launch as well. > > > > > https://codereview.chromium.org/2598113002/diff/1/components/autofill/core/br... > > File components/autofill/core/browser/webdata/autocomplete_sync_bridge.h > > (right): > > > > > https://codereview.chromium.org/2598113002/diff/1/components/autofill/core/br... > > components/autofill/core/browser/webdata/autocomplete_sync_bridge.h:62: // > > Generate a string key uniquely identifying |entity_data| in the context of > > I missed updating this comment. > > From what I can tell, EscapePath > (https://cs.chromium.org/chromium/src/net/base/escape.cc?dr=CSs&sq=package:chr...) > only escapes characters in the kPathCharMap: "#%:<>?[\]^`{|} . I would be > surprised if escaping these characters led to a lot of overhead in practice. My mistake; EscapePath escapes all non-7 bit characters, so this would affect a much wider range of autofill data.
The CQ bit was checked by skym@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...
Okay, I think we should do this. The things I'm considering for changing to protos here are Pros * Deals with escaped characters much more compactly, doesn't cause 3x bloat. This affects EMs that use non-ascii characters, as well as any autofill value that contains spaces. * We don't have to write the reversing logic ourselves, instead use what protobuffers already gives us. * This CL already has unittests, previous approach doesn't. Cons * Minimum overhead increases from 1 to 4 b bytes. * More translations necessary, more stack allocation and copying bytes as we use intermediate objects. Looking at my own personal data, the 1 vs 4 overhead and the spaces must be escaped about cancel each other out.
Okay, I think we should do this. The things I'm considering for changing to protos here are Pros * Deals with escaped characters much more compactly, doesn't cause 3x bloat. This affects EMs that use non-ascii characters, as well as any autofill value that contains spaces. * We don't have to write the reversing logic ourselves, instead use what protobuffers already gives us. * This CL already has unittests, previous approach doesn't. Cons * Minimum overhead increases from 1 to 4 b bytes. * More translations necessary, more stack allocation and copying bytes as we use intermediate objects. Looking at my own personal data, the 1 vs 4 overhead and the spaces must be escaped about cancel each other out.
skym@chromium.org changed reviewers: - gangwu@chromium.org, maxbogue@chromium.org, pnoland@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I noticed you removed me as a reviewer after I started so you're stuck with some comments :P https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... File components/autofill/core/browser/proto/autofill_sync.proto (right): https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/proto/autofill_sync.proto:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017! @_@ https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:165: return GetClientTagFromStrings(specifics.name(), specifics.value()); GetClientTagFromStrings doesn't seem like it'll ever be used anywhere but here, so I'd just leave it inline probably. https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:181: bridge()->GetStorageKey(SpecificsToEntity(AutofillSpecifics()).value()); I don't really think key_empty is adding anything to this test case. I'd just make it test that the keys don't change with timestamps. https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:183: EXPECT_FALSE(key_1_no_time.empty()); Consider replacing all these with a check inside GetStorageKey(). https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:193: EXPECT_NE(key_1_no_time, key_2_no_time); These are pretty redundant with the hard-coded assertions in GetStorageKeyFixed, no?
lgtm % Max's comments https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... File components/autofill/core/browser/proto/autofill_sync.proto (right): https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/proto/autofill_sync.proto:12: // given to Sync to uniquely identify an entity of ModelType syncer::AUTOFILL. Consider "given to Sync" => "passed to sync as storage key". I think mentioning "storage key" makes it clear which function will accept it.
Description was changed from ========== [Sync] Use a proto to generate AutofillSyncStorageKey's storage keys. BUG=675992 ========== to ========== [Sync] Use a proto to generate AutofillSyncStorageKey's storage keys. Previously we were going to use a | delimiter and manually construct storage keys ourselves. The requires us to write logic to reverse it, and is inefficient when encoding characters that need to be escaped. The disadvantages are that we always have 4 bytes (instead of 1) of overhead, and printing the string is less readable. BUG=675992 ==========
skym@chromium.org changed reviewers: + mathp@google.com
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Adding mathp@ for autofill owner. PTAL. https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... File components/autofill/core/browser/proto/autofill_sync.proto (right): https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/proto/autofill_sync.proto:12: // given to Sync to uniquely identify an entity of ModelType syncer::AUTOFILL. On 2017/01/04 18:09:56, pavely wrote: > Consider "given to Sync" => "passed to sync as storage key". > I think mentioning "storage key" makes it clear which function will accept it. Done. https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:165: return GetClientTagFromStrings(specifics.name(), specifics.value()); On 2017/01/04 01:42:46, maxbogue wrote: > GetClientTagFromStrings doesn't seem like it'll ever be used anywhere but here, > so I'd just leave it inline probably. Done. https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:181: bridge()->GetStorageKey(SpecificsToEntity(AutofillSpecifics()).value()); On 2017/01/04 01:42:46, maxbogue wrote: > I don't really think key_empty is adding anything to this test case. I'd just > make it test that the keys don't change with timestamps. Mostly removed. Added a new case comparing empty and \0. https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:183: EXPECT_FALSE(key_1_no_time.empty()); On 2017/01/04 01:42:46, maxbogue wrote: > Consider replacing all these with a check inside GetStorageKey(). Done. https://codereview.chromium.org/2598113002/diff/20001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:193: EXPECT_NE(key_1_no_time, key_2_no_time); On 2017/01/04 01:42:46, maxbogue wrote: > These are pretty redundant with the hard-coded assertions in GetStorageKeyFixed, > no? Removed most of these, and moved some stuff around.
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.
skym@chromium.org changed reviewers: + mathp@chromium.org - mathp@google.com
Whoops, switching to @chromium account for mathp@. PTAL!
sorry for the delay, I really missed this! lgtm with nits https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... File components/autofill/core/browser/proto/BUILD.gn (right): https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... components/autofill/core/browser/proto/BUILD.gn:9: "autofill_sync.proto", why not "sync.proto"? We are already in the autofill namespace/directory https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... File components/autofill/core/browser/proto/autofill_sync.proto (right): https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... components/autofill/core/browser/proto/autofill_sync.proto:15: required string name = 1; I don't particularly like "required" because I believe it crashes on ParseFromString if not complete, but if handled with care I guess I'm OK as there are other instances in Chrome https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:53: std::string GetStorageKeyFromStrings(const std::string& name, How about BuildSerializedStorageKey?
https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... File components/autofill/core/browser/proto/BUILD.gn (right): https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... components/autofill/core/browser/proto/BUILD.gn:9: "autofill_sync.proto", On 2017/01/06 01:51:29, Mathieu Perreault wrote: > why not "sync.proto"? We are already in the autofill namespace/directory You're right, it is redundant. I actually tried to do this when I started. But the include guard that is generated is solely based on the relative path passed into protoc, and I couldn't figure out an easy way to tweak that. So we conflict with the existing sync.proto file. Would be nice if it took into account the proto "package" field when constructing the guard string, oh well. https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... File components/autofill/core/browser/proto/autofill_sync.proto (right): https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... components/autofill/core/browser/proto/autofill_sync.proto:15: required string name = 1; On 2017/01/06 01:51:29, Mathieu Perreault wrote: > I don't particularly like "required" because I believe it crashes on > ParseFromString if not complete, but if handled with care I guess I'm OK as > there are other instances in Chrome I didn't realize that required was generally frowned upon, but the documentation seems to have similar hesitations about required to you. https://developers.google.com/protocol-buffers/docs/proto#specifying-field-rules My thought process is that we're using the serialized to string bytes as our key. We do not want these to vary. And I'm scared that has_x() returning true/false will cause the serialized values to differ. But I think required vs optional just kind of moves around where/how we break when things go wrong. We don't get out of handling with care. So I'm thinking I'll switch to optional and add some DCHECKs verifying that has_value() and has_name() never return false when I add deserialization. Let me know if you don't think this is reasonable. https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2598113002/diff/40001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:53: std::string GetStorageKeyFromStrings(const std::string& name, On 2017/01/06 01:51:29, Mathieu Perreault wrote: > How about BuildSerializedStorageKey? Done.
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2598113002/#ps60001 (title: "Updated for mathp's comments.")
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": 1483727366807550,
"parent_rev": "5a568c2ca3041b4b2b1975508c75d7e95df2a131", "commit_rev":
"ba936951fafeeb20fe1ecb72e3ec40b98bce9fee"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Use a proto to generate AutofillSyncStorageKey's storage keys. Previously we were going to use a | delimiter and manually construct storage keys ourselves. The requires us to write logic to reverse it, and is inefficient when encoding characters that need to be escaped. The disadvantages are that we always have 4 bytes (instead of 1) of overhead, and printing the string is less readable. BUG=675992 ========== to ========== [Sync] Use a proto to generate AutofillSyncStorageKey's storage keys. Previously we were going to use a | delimiter and manually construct storage keys ourselves. The requires us to write logic to reverse it, and is inefficient when encoding characters that need to be escaped. The disadvantages are that we always have 4 bytes (instead of 1) of overhead, and printing the string is less readable. BUG=675992 Review-Url: https://codereview.chromium.org/2598113002 Cr-Commit-Position: refs/heads/master@{#442011} Committed: https://chromium.googlesource.com/chromium/src/+/ba936951fafeeb20fe1ecb72e3ec... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/ba936951fafeeb20fe1ecb72e3ec... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
