[Sync] EntitySpecifics use oneof to save memory |
|||||||
Issue descriptionUse proto2's oneof to reduce memory overhead of EntitySpecifics. Also need to take measurements before and after. Doesn't seem to break any existing testing https://codereview.chromium.org/2650253002/ This shouldn't affect network usage in the slightest, since serialized format should remain unchanged, which is part of why this transition should be so easy. For those with access, see https://sites.google.com/a/google.com/protocol-buffers/user-docs/miscellaneous-howtos/oneof https://docs.google.com/document/d/1dDr2jN0W11P7vet8wWbdLt5CMgWoWAJfBhFV7gJ7fjM/edit#heading=h.nqp0ggt01q98
,
Jan 17 2018
This should be easy and should have real benefits, should be done.
,
Jan 17 2018
Issue 293872 has been merged into this issue.
,
Jan 17 2018
,
Feb 16 2018
Hi, I'm interested in this issue. Can I make a patch? It seems that you already made a patch except a unit test that is commented out. Do you think that the only remained job is making the unit test correct? Or, as you mentioned, is the memory measurement is also necessary? Do you have any suggestion about the memory measurement method?
,
Jul 4
,
Jul 5
Tentatively assigning to feuunk@.
,
Jul 17
As part of this, we should remove the requested_types field. It's the only case where we depend on multiple specifics messages being set in one EntitySpecifics, and it has been deprecated since 2010. This is safe because sync server should not be receiving any requests with RequestedTypes set, as we block any requests from before M18 (for those with access: as of cl/112260616).
,
Jul 17
IIRC we don't support anything older than M28.
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2525f227540a5068818a315e5a0581b49fff4312 commit 2525f227540a5068818a315e5a0581b49fff4312 Author: Florian Uunk <feuunk@chromium.org> Date: Wed Jul 18 10:15:39 2018 Wrap model type specifics in oneof. This should save memory, because all the fields in a oneof share memory, and at most one field can be set at the same time. Also remove the unused requested_types field, that depended on having multiple specifics messages in one entity. Based on https://codereview.chromium.org/2650253002/ BUG= 687426 Change-Id: I83a5b03b222e4ab272bedf41b35cdfd28b4bbc9d Reviewed-on: https://chromium-review.googlesource.com/1129140 Commit-Queue: Florian Uunk <feuunk@chromium.org> Reviewed-by: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#575993} [modify] https://crrev.com/2525f227540a5068818a315e5a0581b49fff4312/components/sync/engine_impl/syncer_proto_util.cc [modify] https://crrev.com/2525f227540a5068818a315e5a0581b49fff4312/components/sync/protocol/proto_value_conversions_unittest.cc [modify] https://crrev.com/2525f227540a5068818a315e5a0581b49fff4312/components/sync/protocol/sync.proto [modify] https://crrev.com/2525f227540a5068818a315e5a0581b49fff4312/components/sync/test/engine/mock_connection_manager.cc
,
Jul 24
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by s...@chromium.org
, Mar 31 2017