sync: remove use of protobuf extensions in protocol to reduce static init overhead.

sync: remove use of protobuf extensions in protocol to reduce static init overhead.

Instead, we now use optional fields in EntitySpecifics.  Because the tag numbers remain
the same, this is a wire-format compatible change.

This incurs a (#datatypes * sizeof(void*))*#sync_items memory cost, due to storing extra pointers.  In practice, for a typical account on windows this amounts to < 200k, and the static init cost is believed to be greater.

Note - upcoming features in protobufs may let us eliminate this extra memory overhead.

Also: The testserver tests were broken on ToT due to a bug in _SaveEntry's saving of mtime which is fixed in this patch.

TBR=yoz@chromium.org
TBR=mnissler@chromium.org
TBR=pkasting@chromium.org
TBR=georgey@chromium.org

BUG=94992, 94925
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124912

[tim (not reviewing), 2012-02-28 05:31:25]

Ready for a peek. I added Nick for chromiumsync.py changes in case his protobuf
fu reveals any niftier ways to accomplish the task. Fred, please review
everything.

A general note: I tried to do this one type at a time, but for a few reasons
this wasn't feasible, ping me if you'd like to discuss.

Also, for good measure (even though it's wire format compatible!) I'm playing
with this against a real server, and in the meantime have been tinkering with a
patch to convert protobuf extensions use server-side to use this new model (I'm
happy to do it, unless server-folks insist ownership.)

[tim (not reviewing), 2012-02-28 05:33:35]

Note: I just remembered I have a pile of comments to update all over the place. 
I'll tackle that first thing tomorrow. Feel free to wait or review keeping that
in mind.

[ncarter (slow), 2012-02-28 18:45:08]

http://codereview.chromium.org/9460047/diff/14001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/9460047/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:9: """
Did you run chromiumsync_test.py? It has pretty good coverage, I would assume
that it would break on a change of this scale.

http://codereview.chromium.org/9460047/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:249: descriptor_handle =
SYNC_TYPE_TO_DESCRIPTOR[data_type]
Drop the "_handle." Just "descriptor." It's cleaner.

[tim (not reviewing), 2012-03-01 22:07:01]

Think this is ready for a good look now! Thanks for patience.

http://codereview.chromium.org/9460047/diff/14001/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/9460047/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:9: """
On 2012/02/28 18:45:09, ncarter wrote:
> Did you run chromiumsync_test.py? It has pretty good coverage, I would assume
> that it would break on a change of this scale.

Done.

http://codereview.chromium.org/9460047/diff/14001/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:249: descriptor_handle =
SYNC_TYPE_TO_DESCRIPTOR[data_type]
On 2012/02/28 18:45:09, ncarter wrote:
> Drop the "_handle." Just "descriptor." It's cleaner.

Done.

[ncarter (slow), 2012-03-01 22:13:05]

Python stuff LGTM with one nit.

http://codereview.chromium.org/9460047/diff/32002/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync_test.py (right):

http://codereview.chromium.org/9460047/diff/32002/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync_test.py:17: from google.protobuf import
text_format
Debugging cruft; remove these lines.

[akalin, 2012-03-02 23:56:49]

LGTM after nits

you may need to add OWNERS

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/proto_value_conversions.cc (right):

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/proto_value_conversions.cc:95: #define
SET_EXTENSION(field, fn)                                    \
line up \s

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/proto_value_conversions.cc:95: #define
SET_EXTENSION(field, fn)                                    \
SET_EXTENSION -> SET_SPECIFICS/SET_FIELD?

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/sync/protoco...
File chrome/browser/sync/protocol/sync.proto (right):

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/sync.proto:59: // definition,define your message
type, and add an optional field
space after first comma

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/sync/protoco...
chrome/browser/sync/protocol/sync.proto:62: // optional MyDatatypeSpecifics
my_datatype = 32222;
two spaces before 'optional'

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/webdata/auto...
File chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

http://codereview.chromium.org/9460047/diff/32002/chrome/browser/webdata/auto...
chrome/browser/webdata/autofill_profile_syncable_service_unittest.cc:211:
.autofill_profile().guid());
indentation

http://codereview.chromium.org/9460047/diff/32002/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/9460047/diff/32002/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync.py:881: entry.mtime =
(int((time.mktime(time.gmtime()) -
what is this for?  mention it in CL desc?

http://codereview.chromium.org/9460047/diff/32002/net/tools/testserver/chromi...
File net/tools/testserver/chromiumsync_test.py (right):

http://codereview.chromium.org/9460047/diff/32002/net/tools/testserver/chromi...
net/tools/testserver/chromiumsync_test.py:17: from google.protobuf import
text_format
not removed yet?

[tim (not reviewing), 2012-03-03 03:09:49]

adding tbr's to cc

thakis, should I be updating perf_expectations.json with this patch or
afterwards?

[Nico, 2012-03-03 03:34:54]

Afterwards.

Thanks for doing this!

(Since our perf infra measures files w static initializers and not
initializers themselves this probably won't be noticed by the perf bots
until pliard's protobuf change lands.)
On Mar 2, 2012 7:09 PM, <tim@chromium.org> wrote:

> adding tbr's to cc
>
> thakis, should I be updating perf_expectations.json with this patch or
> afterwards?
>
>
http://codereview.chromium.**org/9460047/<http://codereview.chromium.org/9460...
>

[Peter Kasting, 2012-03-03 06:37:18]

On 2012/03/03 03:09:49, timsteele wrote:
> adding tbr's to cc

If we're going to be reviewers, even as TBR, please add us as reviewers, not
just CCed people, or these will likely get lost in the mail.

If you're going to add someone as a reviewer, even as an OWNER, even if TBRed,
please say what you want reviewed.  After some peering through the changelist I
think maybe I'm on the list for chrome/browser/search_engines?  Not sure.

[tim (not reviewing), 2012-03-03 21:49:30]

On 2012/03/03 06:37:18, Peter Kasting wrote:
> On 2012/03/03 03:09:49, timsteele wrote:
> > adding tbr's to cc
> 
> If we're going to be reviewers, even as TBR, please add us as reviewers, not
> just CCed people, or these will likely get lost in the mail.
> 
> If you're going to add someone as a reviewer, even as an OWNER, even if TBRed,
> please say what you want reviewed.  After some peering through the changelist
I
> think maybe I'm on the list for chrome/browser/search_engines?  Not sure.

Yeah I should have clarified, sorry about that.

yoz - extensions
georgey - autofill / webdata
mnissler - prefs
pkasting - search_engines

Thanks all.

[Peter Kasting, 2012-03-04 00:49:33]

Thanks!  search_engines LGTM

[commit-bot: I haz the power, 2012-03-04 21:19:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/9460047/36001

[commit-bot: I haz the power, 2012-03-04 23:33:39]

Change committed as 124912

[pliard, 2012-03-05 08:13:53]

Thanks very much for doing that!

On Sat, Mar 3, 2012 at 4:34 AM, Nico Weber <thakis@chromium.org> wrote:

> Afterwards.
>
> Thanks for doing this!
>
> (Since our perf infra measures files w static initializers and not
> initializers themselves this probably won't be noticed by the perf bots
> until pliard's protobuf change lands.)
> On Mar 2, 2012 7:09 PM, <tim@chromium.org> wrote:
>
>> adding tbr's to cc
>>
>> thakis, should I be updating perf_expectations.json with this patch or
>> afterwards?
>>
>>
http://codereview.chromium.**org/9460047/<http://codereview.chromium.org/9460...
>>
>

[Mattias Nissler (ping if slow), 2012-03-12 14:14:14]

LGTM FWIW :)