Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2683203004:
Move Storage for ListValue and DictValue in Union (Closed)
DescriptionMove Storage for ListValue and DictValue in Union
This change moves the storage needed for ListValue and DictionaryValue into the
union of base::Value. In addition, all virtual keywords from Value are dropped.
The purpose of this change is to quickly reduce the memory footprint of
base::Value, which increased after FundamentalValue was inlined.
BUG=646113, 688526
R=brettw@chromium.org
Review-Url: https://codereview.chromium.org/2683203004
Cr-Commit-Position: refs/heads/master@{#451296}
Committed: https://chromium.googlesource.com/chromium/src/+/8e945543e2a82d11b0a2934919ad0ac7ad4e7633
Patch Set 1 #
Total comments: 8
Patch Set 2 : Wrap Dict in Ptr, Move Operators, Remove MockStringValue #Patch Set 3 : Rebase #
Total comments: 6
Patch Set 4 : Implement copy operations, added comments and tests #Patch Set 5 : Add missing return statements. #
Total comments: 7
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by jdoerrie@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...
Hi brettw@, please have a look. https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc File base/values_unittest.cc (left): https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc#old... base/values_unittest.cc:420: }; I had to get rid of this class since Value's constructor is no longer virtual. This unfortunately reduces the quality of the following tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2683203004/diff/1/base/values.h File base/values.h (left): https://codereview.chromium.org/2683203004/diff/1/base/values.h#oldcode361 base/values.h:361: DISALLOW_COPY_AND_ASSIGN(DictionaryValue); Given that it is currently impossible to copy construct or assign DictionaryValues due to the contained unique pointers this probably should have been kept. Also we should probably implement the move constructor and move assignment operator here. https://codereview.chromium.org/2683203004/diff/1/base/values.h#oldcode489 base/values.h:489: DISALLOW_COPY_AND_ASSIGN(ListValue); Same as above. https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc File base/values_unittest.cc (left): https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc#old... base/values_unittest.cc:420: }; On 2017/02/10 15:17:38, jdoerrie wrote: > I had to get rid of this class since Value's constructor is no longer virtual. > This unfortunately reduces the quality of the following tests. The tests are currently failing, because there is another subclass doing some logic in the destructor: |extensions::MockStringValue| (https://codesearch.chromium.org/chromium/src/chrome/browser/extensions/api/pr...) Should tests including the MockStringValue also simply be deleted?
https://codereview.chromium.org/2683203004/diff/1/base/values.h File base/values.h (right): https://codereview.chromium.org/2683203004/diff/1/base/values.h#newcode53 base/values.h:53: using DictStorage = std::map<std::string, std::unique_ptr<Value>>; As I commented in the private thread, does the size of Value go down if this is a unique_ptr<std::map<...>>? If so, I think it's worth doing. https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc File base/values_unittest.cc (left): https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc#old... base/values_unittest.cc:420: }; On 2017/02/10 18:45:44, jdoerrie wrote: > On 2017/02/10 15:17:38, jdoerrie wrote: > > I had to get rid of this class since Value's constructor is no longer virtual. > > This unfortunately reduces the quality of the following tests. > > The tests are currently failing, because there is another subclass doing some > logic in the destructor: > |extensions::MockStringValue| > > (https://codesearch.chromium.org/chromium/src/chrome/browser/extensions/api/pr...) > > Should tests including the MockStringValue also simply be deleted? I think doing so is OK.
The CQ bit was checked by jdoerrie@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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jdoerrie@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/2683203004/diff/1/base/values.h File base/values.h (right): https://codereview.chromium.org/2683203004/diff/1/base/values.h#newcode53 base/values.h:53: using DictStorage = std::map<std::string, std::unique_ptr<Value>>; On 2017/02/11 00:24:04, brettw (plz ping after 24h) wrote: > As I commented in the private thread, does the size of Value go down if this is > a unique_ptr<std::map<...>>? If so, I think it's worth doing. Done. Wrapping the map in a unique_ptr results in sizeof(Value) == 40 for MSVC, Clang and GCC. https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc File base/values_unittest.cc (left): https://codereview.chromium.org/2683203004/diff/1/base/values_unittest.cc#old... base/values_unittest.cc:420: }; On 2017/02/11 00:24:04, brettw (plz ping after 24h) wrote: > On 2017/02/10 18:45:44, jdoerrie wrote: > > On 2017/02/10 15:17:38, jdoerrie wrote: > > > I had to get rid of this class since Value's constructor is no longer > virtual. > > > This unfortunately reduces the quality of the following tests. > > > > The tests are currently failing, because there is another subclass doing some > > logic in the destructor: > > |extensions::MockStringValue| > > > > > (https://codesearch.chromium.org/chromium/src/chrome/browser/extensions/api/pr...) > > > > Should tests including the MockStringValue also simply be deleted? > > I think doing so is OK. Done.
https://codereview.chromium.org/2683203004/diff/40001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2683203004/diff/40001/base/values.cc#newcode502 base/values.cc:502: // Currently not implementable due to the presence of unique_ptrs in I don't quite follow. Shouldn't this be the same as DeepCopy? With the current state of the code, I'm also not sure what prevents us from reaching this code. Before DictionaryValue and ListValue didn't have copy constructors so I believe no code will call this now. But once you check this in it will be easy to convert a DictionaryValue to a Value and then copy that. https://codereview.chromium.org/2683203004/diff/40001/base/values.cc#newcode556 base/values.cc:556: // Currently not implementable due to the presence of unique_ptrs in Same here. https://codereview.chromium.org/2683203004/diff/40001/base/values.h File base/values.h (right): https://codereview.chromium.org/2683203004/diff/40001/base/values.h#newcode184 base/values.h:184: ManualConstructor<std::unique_ptr<DictStorage>> dict_ptr_; Can you add a comment here about why this one is heap-allocated? It will look weirdly random to somebody coming by in 2 years.
The CQ bit was checked by jdoerrie@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...
https://codereview.chromium.org/2683203004/diff/40001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2683203004/diff/40001/base/values.cc#newcode502 base/values.cc:502: // Currently not implementable due to the presence of unique_ptrs in On 2017/02/14 23:15:04, brettw (plz ping after 24h) wrote: > I don't quite follow. Shouldn't this be the same as DeepCopy? With the current > state of the code, I'm also not sure what prevents us from reaching this code. > Before DictionaryValue and ListValue didn't have copy constructors so I believe > no code will call this now. But once you check this in it will be easy to > convert a DictionaryValue to a Value and then copy that. Yes, you are right. When writing this I just considered that |list_.Init(*that.list_);| wouldn't work here. Doing a DeepCopy does work, of course. Changed it now. https://codereview.chromium.org/2683203004/diff/40001/base/values.cc#newcode556 base/values.cc:556: // Currently not implementable due to the presence of unique_ptrs in On 2017/02/14 23:15:05, brettw (plz ping after 24h) wrote: > Same here. Acknowledged. https://codereview.chromium.org/2683203004/diff/40001/base/values.h File base/values.h (right): https://codereview.chromium.org/2683203004/diff/40001/base/values.h#newcode184 base/values.h:184: ManualConstructor<std::unique_ptr<DictStorage>> dict_ptr_; On 2017/02/14 23:15:05, brettw (plz ping after 24h) wrote: > Can you add a comment here about why this one is heap-allocated? It will look > weirdly random to somebody coming by in 2 years. Done. https://codereview.chromium.org/2683203004/diff/80001/base/values.h File base/values.h (left): https://codereview.chromium.org/2683203004/diff/80001/base/values.h#oldcode199 base/values.h:199: ~DictionaryValue() override; Deleting this and the |DISALLOW_COPY_AND_ASSIGN| macro results in copy and move operations to be implicitly declared and implemented by the compiler. Since they are doing the right thing I decided to go for it. However, this also means that they are likely inlined. Given the fact that there are no fields in this class and |Value|'s copy and move operations are defined out of line (and that this class will likely be deleted soon) I decided this is fine. What do you think? https://codereview.chromium.org/2683203004/diff/80001/base/values.h#oldcode375 base/values.h:375: ~ListValue() override; Same here.
lgtm https://codereview.chromium.org/2683203004/diff/80001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2683203004/diff/80001/base/values.cc#newcode500 base/values.cc:500: // DictStorage and ListStorage are move-only types due to the presence of I think this comment is out-of-date now. https://codereview.chromium.org/2683203004/diff/80001/base/values.cc#newcode556 base/values.cc:556: // DictStorage and ListStorage are move-only types due to the presence of Ditto https://codereview.chromium.org/2683203004/diff/80001/base/values.h File base/values.h (left): https://codereview.chromium.org/2683203004/diff/80001/base/values.h#oldcode199 base/values.h:199: ~DictionaryValue() override; Your reasoning sounds right to me.
https://codereview.chromium.org/2683203004/diff/80001/base/values.cc File base/values.cc (right): https://codereview.chromium.org/2683203004/diff/80001/base/values.cc#newcode500 base/values.cc:500: // DictStorage and ListStorage are move-only types due to the presence of On 2017/02/15 21:28:29, brettw (plz ping after 24h) wrote: > I think this comment is out-of-date now. I think it's still relevant. DictStorage and ListStorage are defined like this: using DictStorage = std::map<std::string, std::unique_ptr<Value>>; using ListStorage = std::vector<std::unique_ptr<Value>>; The presence of std::unique_ptr<Value> makes both of them move-only, since they don't have a copy constructor. This is why we currently have to call |list_.Init(std::move(*that.CreateDeepCopy()->list_));| instead of |list_.Init(*that.list_)|. This is different for StringValue and BinaryValue, which is why I added the comment here. However, you are right that it is the proposal's goal to arrive at using DictStorage = std::map<std::string, Value>; using ListStorage = std::vector<Value>; at which point we can simply use the copy constructors and this comment will be obsolete. https://codereview.chromium.org/2683203004/diff/80001/base/values.cc#newcode556 base/values.cc:556: // DictStorage and ListStorage are move-only types due to the presence of On 2017/02/15 21:28:29, brettw (plz ping after 24h) wrote: > Ditto Same as above (s/copy constructor/copy assignment operator)
The CQ bit was checked by jdoerrie@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by jdoerrie@chromium.org
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": 80001, "attempt_start_ts": 1487331837779240,
"parent_rev": "8bbb4f3f8c810f1f178f0fe3a824f99e6333da7c", "commit_rev":
"8e945543e2a82d11b0a2934919ad0ac7ad4e7633"}
Message was sent while issue was closed.
Description was changed from ========== Move Storage for ListValue and DictValue in Union This change moves the storage needed for ListValue and DictionaryValue into the union of base::Value. In addition, all virtual keywords from Value are dropped. The purpose of this change is to quickly reduce the memory footprint of base::Value, which increased after FundamentalValue was inlined. BUG=646113, 688526 R=brettw@chromium.org ========== to ========== Move Storage for ListValue and DictValue in Union This change moves the storage needed for ListValue and DictionaryValue into the union of base::Value. In addition, all virtual keywords from Value are dropped. The purpose of this change is to quickly reduce the memory footprint of base::Value, which increased after FundamentalValue was inlined. BUG=646113, 688526 R=brettw@chromium.org Review-Url: https://codereview.chromium.org/2683203004 Cr-Commit-Position: refs/heads/master@{#451296} Committed: https://chromium.googlesource.com/chromium/src/+/8e945543e2a82d11b0a2934919ad... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/8e945543e2a82d11b0a2934919ad... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
