Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680243003:
Reduce size of mojo AssociatedBinding template expansions (Closed)
|
Created:
3 years, 10 months ago by scottmg Modified:
3 years, 10 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReduce size of mojo AssociatedBinding template expansions
In particular for AssociatedBinding::Bind(), reduces expansion to 54% of
original size on Windows.
This only saves about 4K on the final libchrome.so size on Android, so
not a huge win, but: 1) it doesn't add much complexity to the
implementation, and 2) I think it'll be useful to do more followups that
de-type bindings code.
R=yzshen@chromium.org
BUG=597125, 689690
Review-Url: https://codereview.chromium.org/2680243003
Cr-Commit-Position: refs/heads/master@{#449364}
Committed: https://chromium.googlesource.com/chromium/src/+/83392e9f4312a231f60c5d2b8f0bfa4f80aa92cd
Patch Set 1 #Patch Set 2 : forgot cc #
Total comments: 6
Patch Set 3 : fixes and exports #Patch Set 4 : rebase #
Messages
Total messages: 27 (20 generated)
Description was changed from ========== Reduce size of mojo AssociatedBinding template expansions In particular for AssociatedBinding::Bind(), reduces expansion to 54% of original size on Windows. BUG=597125,689690 ========== to ========== Reduce size of mojo AssociatedBinding template expansions In particular for AssociatedBinding::Bind(), reduces expansion to 54% of original size on Windows. This only saves about 4K on the final libchrome.so size on Android, so not a huge win, but: 1) it doesn't add much complexity to the implementation, and 2) I think it'll be useful to do more followups that de-type bindings code. R=jam@chromium.org BUG=597125,689690 ==========
The CQ bit was checked by scottmg@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 ========== Reduce size of mojo AssociatedBinding template expansions In particular for AssociatedBinding::Bind(), reduces expansion to 54% of original size on Windows. This only saves about 4K on the final libchrome.so size on Android, so not a huge win, but: 1) it doesn't add much complexity to the implementation, and 2) I think it'll be useful to do more followups that de-type bindings code. R=jam@chromium.org BUG=597125,689690 ========== to ========== Reduce size of mojo AssociatedBinding template expansions In particular for AssociatedBinding::Bind(), reduces expansion to 54% of original size on Windows. This only saves about 4K on the final libchrome.so size on Android, so not a huge win, but: 1) it doesn't add much complexity to the implementation, and 2) I think it'll be useful to do more followups that de-type bindings code. R=yzshen@chromium.org BUG=597125,689690 ==========
scottmg@chromium.org changed reviewers: + yzshen@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
LGTM with a few nits. Thanks! https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/associated_binding.cc (right): https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/associated_binding.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. Please put this file in the lib/ directory. https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/associated_binding.cc:29: void AssociatedBindingBase::set_connection_error_handler(const base::Closure& error_handler) { Please run git cl format. https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/associated_binding.h (right): https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/associated_binding.h:153: Interface::HasSyncMethods_, runner, Interface::Version_); nit: std::move(runner) will save one addref/release.
The CQ bit was checked by scottmg@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...
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by scottmg@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...
scottmg@chromium.org changed reviewers: + rockot@chromium.org
Thanks +rockot for ipc/BUILD.gn OWNERS. I'm a little puzzled why that change is necessary, but the error I otherwise get is https://gist.github.com/sgraham/d8ca7852104cdb9d0644509bc15f09cf . https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/associated_binding.cc (right): https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/associated_binding.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/02/09 00:36:52, yzshen1 wrote: > Please put this file in the lib/ directory. Done. https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/associated_binding.cc:29: void AssociatedBindingBase::set_connection_error_handler(const base::Closure& error_handler) { On 2017/02/09 00:36:52, yzshen1 wrote: > Please run git cl format. Oops, done. https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... File mojo/public/cpp/bindings/associated_binding.h (right): https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... mojo/public/cpp/bindings/associated_binding.h:153: Interface::HasSyncMethods_, runner, Interface::Version_); On 2017/02/09 00:36:52, yzshen1 wrote: > nit: std::move(runner) will save one addref/release. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/09 at 01:15:08, scottmg wrote: > Thanks > > +rockot for ipc/BUILD.gn OWNERS. I'm a little puzzled why that change is necessary, but the error I otherwise get is https://gist.github.com/sgraham/d8ca7852104cdb9d0644509bc15f09cf . Yeah at a glance I have no idea how your change could cause this, but you can remove this from your CL since https://codereview.chromium.org/2668153003 adds exactly the same thing for other reasons and it landed last night. > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > File mojo/public/cpp/bindings/associated_binding.cc (right): > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > mojo/public/cpp/bindings/associated_binding.cc:1: // Copyright 2017 The Chromium Authors. All rights reserved. > On 2017/02/09 00:36:52, yzshen1 wrote: > > Please put this file in the lib/ directory. > > Done. > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > mojo/public/cpp/bindings/associated_binding.cc:29: void AssociatedBindingBase::set_connection_error_handler(const base::Closure& error_handler) { > On 2017/02/09 00:36:52, yzshen1 wrote: > > Please run git cl format. > > Oops, done. > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > File mojo/public/cpp/bindings/associated_binding.h (right): > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > mojo/public/cpp/bindings/associated_binding.h:153: Interface::HasSyncMethods_, runner, Interface::Version_); > On 2017/02/09 00:36:52, yzshen1 wrote: > > nit: std::move(runner) will save one addref/release. > > Done.
Patchset #4 (id:80001) has been deleted
On 2017/02/09 17:26:02, Ken Rockot wrote: > On 2017/02/09 at 01:15:08, scottmg wrote: > > Thanks > > > > +rockot for ipc/BUILD.gn OWNERS. I'm a little puzzled why that change is > necessary, but the error I otherwise get is > https://gist.github.com/sgraham/d8ca7852104cdb9d0644509bc15f09cf . > > Yeah at a glance I have no idea how your change could cause this, but you can > remove this from your CL since https://codereview.chromium.org/2668153003 adds > exactly the same thing for other reasons and it landed last night. The magic of the component build. Thanks, done. > > > > > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > > File mojo/public/cpp/bindings/associated_binding.cc (right): > > > > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > > mojo/public/cpp/bindings/associated_binding.cc:1: // Copyright 2017 The > Chromium Authors. All rights reserved. > > On 2017/02/09 00:36:52, yzshen1 wrote: > > > Please put this file in the lib/ directory. > > > > Done. > > > > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > > mojo/public/cpp/bindings/associated_binding.cc:29: void > AssociatedBindingBase::set_connection_error_handler(const base::Closure& > error_handler) { > > On 2017/02/09 00:36:52, yzshen1 wrote: > > > Please run git cl format. > > > > Oops, done. > > > > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > > File mojo/public/cpp/bindings/associated_binding.h (right): > > > > > https://codereview.chromium.org/2680243003/diff/20001/mojo/public/cpp/binding... > > mojo/public/cpp/bindings/associated_binding.h:153: Interface::HasSyncMethods_, > runner, Interface::Version_); > > On 2017/02/09 00:36:52, yzshen1 wrote: > > > nit: std::move(runner) will save one addref/release. > > > > Done.
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org Link to the patchset: https://codereview.chromium.org/2680243003/#ps100001 (title: "rebase")
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": 100001, "attempt_start_ts": 1486661552805160,
"parent_rev": "24ad321a9929fc683c1af2bbfd47ed0d6b015886", "commit_rev":
"83392e9f4312a231f60c5d2b8f0bfa4f80aa92cd"}
Message was sent while issue was closed.
Description was changed from ========== Reduce size of mojo AssociatedBinding template expansions In particular for AssociatedBinding::Bind(), reduces expansion to 54% of original size on Windows. This only saves about 4K on the final libchrome.so size on Android, so not a huge win, but: 1) it doesn't add much complexity to the implementation, and 2) I think it'll be useful to do more followups that de-type bindings code. R=yzshen@chromium.org BUG=597125,689690 ========== to ========== Reduce size of mojo AssociatedBinding template expansions In particular for AssociatedBinding::Bind(), reduces expansion to 54% of original size on Windows. This only saves about 4K on the final libchrome.so size on Android, so not a huge win, but: 1) it doesn't add much complexity to the implementation, and 2) I think it'll be useful to do more followups that de-type bindings code. R=yzshen@chromium.org BUG=597125,689690 Review-Url: https://codereview.chromium.org/2680243003 Cr-Commit-Position: refs/heads/master@{#449364} Committed: https://chromium.googlesource.com/chromium/src/+/83392e9f4312a231f60c5d2b8f0b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as https://chromium.googlesource.com/chromium/src/+/83392e9f4312a231f60c5d2b8f0b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
