Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1708183002:
WTF::bind: Properly handle movable objects in bound arguments. (Closed)
|
Created:
4 years, 10 months ago by Yuta Kitamura Modified:
4 years, 10 months ago CC:
blink-reviews, blink-reviews-wtf_chromium.org, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews_chromium.org, Mikhail, tyoshino+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWTF::bind: Properly handle movable objects in bound arguments.
This patch makes WTF::bind correctly handle movable objects all the way
of the bind() call, including wrapping and unwrapping of arguments.
With this patch, you can pass move-only objects to bound arguments
and they are handled efficiently.
The trickiest part is that the arguments are passed across a function
template, which is blessed with C++'s universal reference, and a class
template, which is not. Because of that, PartBoundFunctionImpl<> must be
instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs
to use a correct version of ParamStorageTraits.
The unbound parameters are untouched in this patch. CrossThreadCopier
isn't changed, either. Therefore threadSafeBind() always forces a copy
for bound arguments for now.
BUG=565765
R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org
Committed: https://crrev.com/fbbd4c94d77ab61145beff792e4adbe6c3239582
Cr-Commit-Position: refs/heads/master@{#377837}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Not using using; use std::decay; add missing #include. #Patch Set 3 : Cut the unnecessary copy; add more tests. #Patch Set 4 : Unwrap now returns non-const lref; add moar comments. #
Messages
Total messages: 39 (13 generated)
The CQ bit was checked by yutak@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/1708183002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708183002/1
PTAL (I didn't check with Windows so there may be issues in trybots, though...)
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_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:165: using StorageTraits = ParamStorageTraits<typename std::conditional<std::is_reference<T>::value, typename std::remove_cv<typename std::remove_reference<T>::type>::type, T>::type>; I think this hits MSVC2013 bug. "using" is so buggy on it. Could you try to use struct here instead of using?
https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:64: R operator()(Params... params) Can we use Universal References here? # It's not necessary to do in this CL. Params can probably be passed by value and there's an unnecessary copy here.
https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:165: using StorageTraits = ParamStorageTraits<typename std::conditional<std::is_reference<T>::value, typename std::remove_cv<typename std::remove_reference<T>::type>::type, T>::type>; Can we apply std::remove_cv unconditionally here, or just std::decay? It doesn't make sense to me to store it in cv-qualified form. Also, StorageTraits is always used for BoundParameters and all items in it are l-ref or r-ref. https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:178: return callInternal(unbound..., base::MakeIndexSequence<sizeof...(BoundParameters)>()); # It's not necessary to do in this CL, again. Can unbound... be std::forward<UnboundParameters>(unbound)... here and line 186? Also, can UnboundParameters.. at line 183 be UnboundParameters&&...? So that we can potentially reduce a copy.
Drive-by. https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:66: return m_function(std::forward<Params>(params)...); Nit: #include <utility> https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/FunctionalTest.cpp (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/FunctionalTest.cpp:356: MoveOnly(std::move(moveOnly)); // Consume the object. Nit: #include <utility>
Description was changed from ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ========== to ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. Note that, after this patch, calling a bound function twice causes a different result, since, if there is a bound value that was moved in, it is moved out in the first invocation of the function. There are no cases where we call the bound function twice or more AFAIK, so this should be fine. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ==========
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
PTAL at PS2 https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:64: R operator()(Params... params) On 2016/02/18 14:49:18, tzik wrote: > Can we use Universal References here? > # It's not necessary to do in this CL. > > Params can probably be passed by value and there's an unnecessary copy here. No, we don't have to do that, because Params now include lref or rref as necessary. (If there was a copy here, my unit test with MoveOnly should fail to compile.) https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:66: return m_function(std::forward<Params>(params)...); On 2016/02/18 17:11:13, dcheng wrote: > Nit: #include <utility> Done. https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:165: using StorageTraits = ParamStorageTraits<typename std::conditional<std::is_reference<T>::value, typename std::remove_cv<typename std::remove_reference<T>::type>::type, T>::type>; On 2016/02/18 15:15:32, tzik wrote: > Can we apply std::remove_cv unconditionally here, or just std::decay? > It doesn't make sense to me to store it in cv-qualified form. > Also, StorageTraits is always used for BoundParameters and all items in it are > l-ref or r-ref. std::decay seems to work, so I'm going with that path. https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:178: return callInternal(unbound..., base::MakeIndexSequence<sizeof...(BoundParameters)>()); On 2016/02/18 15:15:32, tzik wrote: > # It's not necessary to do in this CL, again. > Can unbound... be std::forward<UnboundParameters>(unbound)... here and line 186? > Also, can UnboundParameters.. at line 183 be UnboundParameters&&...? So that we > can potentially reduce a copy. I'm planning to improve unbound parameters' situation in another CL. https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/FunctionalTest.cpp (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/FunctionalTest.cpp:356: MoveOnly(std::move(moveOnly)); // Consume the object. On 2016/02/18 17:11:13, dcheng wrote: > Nit: #include <utility> Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708183002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708183002/20001
https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:115: static T unwrap(StorageType& value) { return std::move(value); } Turned out I need to consider the case when StorageType's move constructor is explicitly deleted. Hmm.
https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:115: static T unwrap(StorageType& value) { return std::move(value); } On 2016/02/19 08:02:13, Yuta Kitamura wrote: > Turned out I need to consider the case when StorageType's > move constructor is explicitly deleted. Hmm. Seems like non-movable copyable classes are almost unusable (it's not possible to return an rvalue object), so I'm gonna ignore this.
Description was changed from ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. Note that, after this patch, calling a bound function twice causes a different result, since, if there is a bound value that was moved in, it is moved out in the first invocation of the function. There are no cases where we call the bound function twice or more AFAIK, so this should be fine. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ========== to ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. Note that, after this patch, calling a bound function twice causes a different result, since, if there is a movable bound argument, it is moved out on the first invocation of the function. There are no cases where we call the bound function twice or more AFAIK, so this should be fine. (We could programatically forbid that?) BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ==========
The CQ bit was checked by yutak@chromium.org to run a CQ dry run
PTAL at PS3, I think I've removed all the necessary copies.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708183002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708183002/40001
https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/Functional.h (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/Functional.h:64: R operator()(Params... params) On 2016/02/19 06:59:50, Yuta Kitamura wrote: > On 2016/02/18 14:49:18, tzik wrote: > > Can we use Universal References here? > > # It's not necessary to do in this CL. > > > > Params can probably be passed by value and there's an unnecessary copy here. > > No, we don't have to do that, because Params now include > lref or rref as necessary. (If there was a copy here, my > unit test with MoveOnly should fail to compile.) As we chatted locally, if it's movable then it'll be moved. Though when it's copyable but not efficiently movable and passed by value, it causes an extra copy that we can remove for free. https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/FunctionalTest.cpp (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/FunctionalTest.cpp:353: int singleMoveOnly(MoveOnly&& moveOnly) Could you add another test case for pass-by-value?
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...)
Description was changed from ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. Note that, after this patch, calling a bound function twice causes a different result, since, if there is a movable bound argument, it is moved out on the first invocation of the function. There are no cases where we call the bound function twice or more AFAIK, so this should be fine. (We could programatically forbid that?) BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ========== to ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. With this patch, you can pass move-only objects to bound arguments and they are handled efficiently. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ==========
The CQ bit was checked by yutak@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/1708183002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708183002/60001
According to offline discussion, we decided to avoid automatic move-out on unwrap(), so the functor can be called multiple times. To do that, I changed the return type of unwrap() to non- const lvalue reference. (You can still manually move out by applying std::move() to that lvalue reference.) PTAL? https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/wtf/FunctionalTest.cpp (right): https://codereview.chromium.org/1708183002/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/wtf/FunctionalTest.cpp:353: int singleMoveOnly(MoveOnly&& moveOnly) On 2016/02/19 10:42:45, tzik wrote: > Could you add another test case for pass-by-value? This was added, but removed in the end because we decided to not do automatic move-out on unwrap().
lgtm
owner LGTM (relying on tzik's one).
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_...)
I'm landing this now. hiroshige: Pls speak up if you find something concerning even after it's landed.
The CQ bit was checked by yutak@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1708183002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1708183002/60001
lgtm.
lgtm.
Message was sent while issue was closed.
Description was changed from ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. With this patch, you can pass move-only objects to bound arguments and they are handled efficiently. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ========== to ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. With this patch, you can pass move-only objects to bound arguments and they are handled efficiently. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. With this patch, you can pass move-only objects to bound arguments and they are handled efficiently. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org ========== to ========== WTF::bind: Properly handle movable objects in bound arguments. This patch makes WTF::bind correctly handle movable objects all the way of the bind() call, including wrapping and unwrapping of arguments. With this patch, you can pass move-only objects to bound arguments and they are handled efficiently. The trickiest part is that the arguments are passed across a function template, which is blessed with C++'s universal reference, and a class template, which is not. Because of that, PartBoundFunctionImpl<> must be instantiated with BoundParameters&&, and PartBoundFunctionImpl<> needs to use a correct version of ParamStorageTraits. The unbound parameters are untouched in this patch. CrossThreadCopier isn't changed, either. Therefore threadSafeBind() always forces a copy for bound arguments for now. BUG=565765 R=haraken@chromium.org, hiroshige@chromium.org, tzik@chromium.org Committed: https://crrev.com/fbbd4c94d77ab61145beff792e4adbe6c3239582 Cr-Commit-Position: refs/heads/master@{#377837} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fbbd4c94d77ab61145beff792e4adbe6c3239582 Cr-Commit-Position: refs/heads/master@{#377837} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
