Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1709483002:
Support move-only type on base::Callback::Run (Closed)
DescriptionSupport move-only type on base::Callback::Run
Change base::Callback<R(Args...)>::Run argument signature from
CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the
arguments in the rest of the callchain to the bound function.
This implies making an extra copy when a pass-by-value argument of
Callback::Run() is a copyable-but-not-efficiently-movable types.
That is due to broken Perfect Forwarding from the callchain from
Callback::Run() to the bound function. However, Perfect Forwarding
idiom is not fully applicable to the callchain, since we have to
determine the type of a function pointer in the Callback ctor,
before Callback::Run() is called.
Also, this CL converts a copy construction to a move construction
when a pass-by-value argument of Callback::Run() is a general movable
type. E.g. std::string, std::map and others movable class without
DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND.
BUG=554299
Committed: https://crrev.com/a43eff01d5024ee6722ebd637c30890681fbfe21
Cr-Commit-Position: refs/heads/master@{#380080}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : rebase #Patch Set 8 : remove Passed() whitelist check #
Total comments: 9
Patch Set 9 : +comment #
Total comments: 6
Patch Set 10 : #
Messages
Total messages: 67 (32 generated)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/1
Description was changed from ========== Support move-only type on base::Callback::Run BUG= ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/20001
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. BUG= ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. BUG=554299 ==========
tzik@chromium.org changed reviewers: + dcheng@chromium.org, thakis@chromium.org
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: media/filters/chunk_demuxer.cc change is decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: media/filters/chunk_demuxer.cc change is decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: //media changes are decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/40001
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/100001
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/120001
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. This enables to pass a movable arguhent to the receiver without whitelisting. This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: //media changes are decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. Pros: This enables to pass a movable arguhent to the receiver without whitelisting. Cons: This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: //media changes are decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ==========
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. Pros: This enables to pass a movable arguhent to the receiver without whitelisting. Cons: This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: //media changes are decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. Pros: This enables to pass a movable arguhent to the receiver without whitelisting. Cons: This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: Non-//base changes are decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. Pros: This enables to pass a movable arguhent to the receiver without whitelisting. Cons: This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. # Note: Non-//base changes are decoupled to http://crrev.com/1704873002. # This CL includes it for try job. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. Pros: This enables to pass a movable arguhent to the receiver without whitelisting. Cons: This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. BUG=554299 ==========
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Does this mean behavior is going to differ between early-bound and late-bound arguments? Will we end up with a situation like this? Bind(&F, Passed(&non_whitelisted_moveonly_type)).Run(); // won't work Bind(&F).Run(std::move(&non_whitelisted_move_only_type)); // works
On 2016/02/22 18:18:21, dcheng wrote: > Does this mean behavior is going to differ between early-bound and late-bound > arguments? Will we end up with a situation like this? > > Bind(&F, Passed(&non_whitelisted_moveonly_type)).Run(); // won't work > Bind(&F).Run(std::move(&non_whitelisted_move_only_type)); // works Yes, they differ at this point. I'll propose to remove the whitelist and to make Passed() accept any type in a next CL. The former early-bound case will work eventually.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
On 2016/02/22 20:14:04, tzik wrote: > On 2016/02/22 18:18:21, dcheng wrote: > > Does this mean behavior is going to differ between early-bound and late-bound > > arguments? Will we end up with a situation like this? > > > > Bind(&F, Passed(&non_whitelisted_moveonly_type)).Run(); // won't work > > Bind(&F).Run(std::move(&non_whitelisted_move_only_type)); // works > > Yes, they differ at this point. > I'll propose to remove the whitelist and to make Passed() accept any type in a > next CL. > The former early-bound case will work eventually. I've put it in this CL. So both should work now.
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
tzik@chromium.org changed reviewers: + danakj@chromium.org
+danakj@. dcheng@, thakis@: Could you take a look? Do you have any comment?
From my recollection of hacking on this, the extra copies seem unavoidable, but I have two questions: - Does this have any effect on binary size? =) - Any idea how often we end up copying twice now? Will this have any visible perf impact?
On 2016/03/01 02:05:47, dcheng wrote: > From my recollection of hacking on this, the extra copies seem unavoidable, but > I have two questions: > - Does this have any effect on binary size? =) > - Any idea how often we end up copying twice now? Will this have any visible > perf impact? On 2016/03/01 02:05:47, dcheng wrote: > From my recollection of hacking on this, the extra copies seem unavoidable, but > I have two questions: > - Does this have any effect on binary size? =) > - Any idea how often we end up copying twice now? Will this have any visible > perf impact? The stripped binary size on Linux loses 8kB! I expect this CL will not cause any visible regression. On my scan for extra copy, all Callback::Run callers use either - lvalue-reference, rvalue-reference, scalar types, - IsMoveOnlyType<T>::value == true, - scoped_refptr, std::vector, std::map, std::string, - base::Time, base::TimeDelta, base::TimeTicks, - base::FileDescriptor, base::Callback - mojo::String, mojo::InlinedStructPtr, mojo::StructPtr, mojo::Handle, - v8::Local, gin::Handle<>, - gfx::Size, gpu::IdType<>, - media::PipelineMetadata, - net::WriteResult, - ProfileStatValue - MemoryUsageStats - ppapi::PepperFileIOHost::UIThreadStuff - GURL in //chrome/browser/safe_browsing/client_side_detection_service.h - cc::SurfaceSequence, cc::SurfaceId, content::DesktopMediaID, invalidation::Status, - content::DevToolsCommandId, history::HistoryCountResult, - syncer::EnumSet, - policy::PolicyLoadResult, syncer::SyncError, base::WeakPtr<>, - remoting::protocol::PairingRegistry::Pairing Hmm, the list has grown larger than I expect... Anyway, assuming their move-ctors are light enough, this CL does not affect the overall performance. I think, some classes in the list should be support move-construction or should be passed by const-ref: - policy::PolicyLoadResult, - syncer::SyncError, - media::PipelineMetadata, - ppapi::PepperFileIOHost::UIThreadStuff, - GURL, - WeakPtr<>
https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h#ne... base/bind_helpers.h:566: typename std::enable_if<!std::is_lvalue_reference<T>::value>::type* = Is it worth trying to catch that the type is movable? Does Passed(ReturnCopyOnlyObject()) compile? https://codereview.chromium.org/1709483002/diff/160001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/bind_internal.h#n... base/bind_internal.h:347: static R Run(BindStateBase* base, UnboundArgs&&... unbound_args) { Just thinking out loud: what happens if we have: template<typename... RunArgs> static R Run(BindStateBase* base, RunArgs&& run_args) { return InvokeHelperType::MakeItSo(..., std::forward<RunArgs>(run_args)...); } I guess maybe that doesn't get us anything, since we already have UnboundArgs&&, and it's more complicated… ok so I think this is fine. https://codereview.chromium.org/1709483002/diff/160001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/callback.h#newcod... base/callback.h:389: R Run(Args... args) const { So this costs an extra copy now, since we don't have enough information to do perfect forwarding: this was determined when we instantiated the callback, when we don't yet know how Run() will be called. We can't make this use forwarding references, because we need to instantiate a reference to this in the ctor. So this is the best we can do. (Maybe we could have a comment in the CL description or in the code or in both that documents why the seemingly-obvious perfect forwarding pattern doesn't work here?)
https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h#ne... base/bind_helpers.h:566: typename std::enable_if<!std::is_lvalue_reference<T>::value>::type* = On 2016/03/02 00:25:45, dcheng wrote: > Is it worth trying to catch that the type is movable? Does > Passed(ReturnCopyOnlyObject()) compile? I think there's no good way to detect the type is movable but not copyable. Sure, it's detectable if its move-constructor is explicitly deleted, but if it just has a copy-ctor and doesn't have move-ctor, std::is_move_constructible returns true. And, the deleted move-constructor on a copyable type doesn't semantically make sense to me. https://codereview.chromium.org/1709483002/diff/160001/base/bind_internal.h File base/bind_internal.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/bind_internal.h#n... base/bind_internal.h:347: static R Run(BindStateBase* base, UnboundArgs&&... unbound_args) { On 2016/03/02 00:25:45, dcheng wrote: > Just thinking out loud: what happens if we have: > > template<typename... RunArgs> > static R Run(BindStateBase* base, RunArgs&& run_args) { > return InvokeHelperType::MakeItSo(..., std::forward<RunArgs>(run_args)...); > } > > I guess maybe that doesn't get us anything, since we already have UnboundArgs&&, > and it's more complicated… ok so I think this is fine. Hmm, since we need to take a function pointer to Invoker<>::Run in Callback, we need to instantiate it earlier than Callback::Run is called like: PolymorphicInvoke invoke_func = &internal::BindState<>::InvokerType::Run<Args...>; in Callback::Callback(). This will make Invoker simpler a bit, and will work on gcc and cl.exe. Not sure it works on clang. Anyway, let me try it on a separate CL. https://codereview.chromium.org/1709483002/diff/160001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/callback.h#newcod... base/callback.h:389: R Run(Args... args) const { On 2016/03/02 00:25:45, dcheng wrote: > So this costs an extra copy now, since we don't have enough information to do > perfect forwarding: this was determined when we instantiated the callback, when > we don't yet know how Run() will be called. > > We can't make this use forwarding references, because we need to instantiate a > reference to this in the ctor. So this is the best we can do. > > (Maybe we could have a comment in the CL description or in the code or in both > that documents why the seemingly-obvious perfect forwarding pattern doesn't work > here?) Acknowledged. Updating.
https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h#ne... base/bind_helpers.h:566: typename std::enable_if<!std::is_lvalue_reference<T>::value>::type* = On 2016/03/02 at 01:04:21, tzik wrote: > On 2016/03/02 00:25:45, dcheng wrote: > > Is it worth trying to catch that the type is movable? Does > > Passed(ReturnCopyOnlyObject()) compile? > > I think there's no good way to detect the type is movable but not copyable. > Sure, it's detectable if its move-constructor is explicitly deleted, but if it just has a copy-ctor and doesn't have move-ctor, std::is_move_constructible returns true. > And, the deleted move-constructor on a copyable type doesn't semantically make sense to me. FWIW, I don't think it's that hard to end up with a copyable but not movable class: won't this be the case for any struct/class with const fields? However, I guess this is fine: std::move() already can't warn if you call it on something with no move constructor, so this should be something that developers already know about.
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the receiver. Pros: This enables to pass a movable arguhent to the receiver without whitelisting. Cons: This increases the number of copies from one to two when a non-movable and copyable type is passed by value to a destination, and also increases the number of the move construction from zero to one when a movable type is passed by value. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the bound function. This implies making an extra copy when a pass-by-value argument of the Callback::Run() is a copyable-but-not-efficiently-movable types. That is due to broken Perfect Forwarding from the callchain from Callback::Run() to the bound function. However, Perfect Forwarding idiom is not fully applicable to the callchain, since we have to determine the type of a function pointer in the Callback ctor, before Callback::Run() is called. BUG=554299 ==========
https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h File base/bind_helpers.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/bind_helpers.h#ne... base/bind_helpers.h:566: typename std::enable_if<!std::is_lvalue_reference<T>::value>::type* = On 2016/03/02 08:06:51, dcheng wrote: > On 2016/03/02 at 01:04:21, tzik wrote: > > On 2016/03/02 00:25:45, dcheng wrote: > > > Is it worth trying to catch that the type is movable? Does > > > Passed(ReturnCopyOnlyObject()) compile? > > > > I think there's no good way to detect the type is movable but not copyable. > > Sure, it's detectable if its move-constructor is explicitly deleted, but if it > just has a copy-ctor and doesn't have move-ctor, std::is_move_constructible > returns true. > > And, the deleted move-constructor on a copyable type doesn't semantically make > sense to me. > > FWIW, I don't think it's that hard to end up with a copyable but not movable > class: won't this be the case for any struct/class with const fields? > > However, I guess this is fine: std::move() already can't warn if you call it on > something with no move constructor, so this should be something that developers > already know about. Ah sorry, I told something incorrect. I meant, it's hard to detect if a copyable class is efficiently movable. If a class has a copy-ctor but doesn't have a move-ctor, its move-construction is fallback to a copy. Such a class is detected as movable, even if it's not lightweight. But, I think we can't detect this and reject it as a CopyOnly object. https://codereview.chromium.org/1709483002/diff/160001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1709483002/diff/160001/base/callback.h#newcod... base/callback.h:389: R Run(Args... args) const { On 2016/03/02 00:25:45, dcheng wrote: > So this costs an extra copy now, since we don't have enough information to do > perfect forwarding: this was determined when we instantiated the callback, when > we don't yet know how Run() will be called. > > We can't make this use forwarding references, because we need to instantiate a > reference to this in the ctor. So this is the best we can do. > > (Maybe we could have a comment in the CL description or in the code or in both > that documents why the seemingly-obvious perfect forwarding pattern doesn't work > here?) Done.
LGTM, just a few comment suggestions. https://codereview.chromium.org/1709483002/diff/180001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1709483002/diff/180001/base/callback.h#newcod... base/callback.h:390: // if an item of |args| is passed-by-value and a copyable-but-not-movable. Nit: Run() makes an extra copy compared to directly calling the bound function if an argument is passed-by-value and is copyable-but-not-movable: https://codereview.chromium.org/1709483002/diff/180001/base/callback.h#newcod... base/callback.h:391: // I.e. below copies CopyableNonMovableType twice. Nit: i.e. https://codereview.chromium.org/1709483002/diff/180001/base/callback.h#newcod... base/callback.h:398: // of |f| before we know how the arguments of Callback::Run() are passed. Nit: Perfect Forwarding requires knowing how the caller will pass the arguments. However, the signature of Run() needs to be fixed in the callback constructor, so Run() cannot template its arguments based on how it's called.
https://codereview.chromium.org/1709483002/diff/180001/base/callback.h File base/callback.h (right): https://codereview.chromium.org/1709483002/diff/180001/base/callback.h#newcod... base/callback.h:390: // if an item of |args| is passed-by-value and a copyable-but-not-movable. On 2016/03/02 23:00:57, dcheng wrote: > Nit: Run() makes an extra copy compared to directly calling the bound function > if an argument is passed-by-value and is copyable-but-not-movable: Done. https://codereview.chromium.org/1709483002/diff/180001/base/callback.h#newcod... base/callback.h:391: // I.e. below copies CopyableNonMovableType twice. On 2016/03/02 23:00:57, dcheng wrote: > Nit: i.e. Done. https://codereview.chromium.org/1709483002/diff/180001/base/callback.h#newcod... base/callback.h:398: // of |f| before we know how the arguments of Callback::Run() are passed. On 2016/03/02 23:00:57, dcheng wrote: > Nit: Perfect Forwarding requires knowing how the caller will pass the arguments. > However, the signature of Run() needs to be fixed in the callback constructor, > so Run() cannot template its arguments based on how it's called. Done, but slightly modified. We can template Callback::Run() to use Universal References with some caller side change (e.g. NULL to nullptr rewrite and to avoid to take a function pointer to Callback::Run), though it makes Run() impl very complicated and doesn't reduce any copy.
The CQ bit was checked by tzik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
mojo::String and maybe MemoryUsageStats sound possibly a bit worrying. Should the places that pass them around pass them differently? This lgtm, thanks for the good discussion on this review. Maybe mention in the CL description that in practice, this removes copy ctor calls for movable types, so it's not just additional copy ctors, but more a different mix. Please do watch out for perf alerts after landing this.
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the bound function. This implies making an extra copy when a pass-by-value argument of the Callback::Run() is a copyable-but-not-efficiently-movable types. That is due to broken Perfect Forwarding from the callchain from Callback::Run() to the bound function. However, Perfect Forwarding idiom is not fully applicable to the callchain, since we have to determine the type of a function pointer in the Callback ctor, before Callback::Run() is called. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the bound function. This implies making an extra copy when a pass-by-value argument of Callback::Run() is a copyable-but-not-efficiently-movable types. That is due to broken Perfect Forwarding from the callchain from Callback::Run() to the bound function. However, Perfect Forwarding idiom is not fully applicable to the callchain, since we have to determine the type of a function pointer in the Callback ctor, before Callback::Run() is called. Also, this CL converts a copy construction to a move construction when a pass-by-value argument of Callback::Run() is a general movable type. E.g. std::string, std::map and others movable class without DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND. BUG=554299 ==========
On 2016/03/08 20:24:12, Nico wrote: > mojo::String and maybe MemoryUsageStats sound possibly a bit worrying. Should > the places that pass them around pass them differently? > > This lgtm, thanks for the good discussion on this review. Maybe mention in the > CL description that in practice, this removes copy ctor calls for movable types, > so it's not just additional copy ctors, but more a different mix. > > Please do watch out for perf alerts after landing this. IMO, we can easily convert passed-by-value MemoryUsageStats to const-ref. Filed a issue for mojo::String, http://crbug.com/593220. mojo::String can/should be movable. Updated the CL description for movable type and move construction.
The CQ bit was checked by tzik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/1709483002/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709483002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709483002/200001
Message was sent while issue was closed.
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the bound function. This implies making an extra copy when a pass-by-value argument of Callback::Run() is a copyable-but-not-efficiently-movable types. That is due to broken Perfect Forwarding from the callchain from Callback::Run() to the bound function. However, Perfect Forwarding idiom is not fully applicable to the callchain, since we have to determine the type of a function pointer in the Callback ctor, before Callback::Run() is called. Also, this CL converts a copy construction to a move construction when a pass-by-value argument of Callback::Run() is a general movable type. E.g. std::string, std::map and others movable class without DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the bound function. This implies making an extra copy when a pass-by-value argument of Callback::Run() is a copyable-but-not-efficiently-movable types. That is due to broken Perfect Forwarding from the callchain from Callback::Run() to the bound function. However, Perfect Forwarding idiom is not fully applicable to the callchain, since we have to determine the type of a function pointer in the Callback ctor, before Callback::Run() is called. Also, this CL converts a copy construction to a move construction when a pass-by-value argument of Callback::Run() is a general movable type. E.g. std::string, std::map and others movable class without DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND. BUG=554299 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the bound function. This implies making an extra copy when a pass-by-value argument of Callback::Run() is a copyable-but-not-efficiently-movable types. That is due to broken Perfect Forwarding from the callchain from Callback::Run() to the bound function. However, Perfect Forwarding idiom is not fully applicable to the callchain, since we have to determine the type of a function pointer in the Callback ctor, before Callback::Run() is called. Also, this CL converts a copy construction to a move construction when a pass-by-value argument of Callback::Run() is a general movable type. E.g. std::string, std::map and others movable class without DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND. BUG=554299 ========== to ========== Support move-only type on base::Callback::Run Change base::Callback<R(Args...)>::Run argument signature from CallbackParamTraits<Args>::ForwardType to bare Arg, and forward the arguments in the rest of the callchain to the bound function. This implies making an extra copy when a pass-by-value argument of Callback::Run() is a copyable-but-not-efficiently-movable types. That is due to broken Perfect Forwarding from the callchain from Callback::Run() to the bound function. However, Perfect Forwarding idiom is not fully applicable to the callchain, since we have to determine the type of a function pointer in the Callback ctor, before Callback::Run() is called. Also, this CL converts a copy construction to a move construction when a pass-by-value argument of Callback::Run() is a general movable type. E.g. std::string, std::map and others movable class without DISALLOW_COPY_AND_ASSIGN_WITH_MOVE_FOR_BIND. BUG=554299 Committed: https://crrev.com/a43eff01d5024ee6722ebd637c30890681fbfe21 Cr-Commit-Position: refs/heads/master@{#380080} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a43eff01d5024ee6722ebd637c30890681fbfe21 Cr-Commit-Position: refs/heads/master@{#380080} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
