|
|
Created:
4 years, 3 months ago by loyso (OOO) Modified:
4 years, 2 months ago Reviewers:
dcheng CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBase ObserverList: Add basic support for standard C++ iterators.
Also add a support for const_iterator.
The range-based for loop support follows for free.
See the use case in the dependent CL.
BUG=634916
Committed: https://crrev.com/29025b60361ec76cf02bab720703c66ae8047be9
Cr-Commit-Position: refs/heads/master@{#424381}
Patch Set 1 #Patch Set 2 : Remove ValueType. #Patch Set 3 : Fix gcc and msvc. #
Total comments: 1
Patch Set 4 : Erase postfix ++ version. Add comments on deprecation. #
Total comments: 4
Patch Set 5 : Fix code review issues. #Patch Set 6 : An attempt to fix strange Win7 behavior. #Patch Set 7 : Revert it to be as in patch set 4 to check win7. #Patch Set 8 : Revert it to be as in patch set 4 to understand win7. #Patch Set 9 : DCHECK in operator-> #Patch Set 10 : Add the optimization change. #Patch Set 11 : Add a comment on MSVC. #
Total comments: 10
Patch Set 12 : Add explicit zeroes in ctor's member initializors. #Patch Set 13 : Address code review issues. #Patch Set 14 : Some usages expect macro to be an expression, not a statement. #
Total comments: 11
Patch Set 15 : Just rebase. #Patch Set 16 : Revert GetCurrent (wm_unittests start to fail) #Patch Set 17 : Fix bugs. Add new tests. #
Total comments: 2
Patch Set 18 : Rename GetNonNullIndex. Add more tests. #Patch Set 19 : Remove excessive mutable. #
Messages
Total messages: 82 (49 generated)
The CQ bit was checked by loyso@chromium.org to run a CQ dry run
Description was changed from ========== Base ObserverList: Add basic support for standadrd C++ iterators. See the use case in the dependent CL. BUG=634916 ========== to ========== Base ObserverList: Add basic support for standadrd C++ iterators. See the use case in the dependent CL. BUG=634916 ==========
loyso@chromium.org changed reviewers: + thakis@chromium.org
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 ========== Base ObserverList: Add basic support for standadrd C++ iterators. See the use case in the dependent CL. BUG=634916 ========== to ========== Base ObserverList: Add basic support for standard C++ iterators. Also add a support for const_iterator. The range-based for loop support follows for free. See the use case in the dependent CL. BUG=634916 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Do not review please - to be fixed.
The CQ bit was checked by loyso@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL! It's ready to be landed.
loyso@chromium.org changed reviewers: + dcheng@chromium.org
I haven't reviewed the entire patch yet, but can we completely remove the old FOR_EACH_OBSERVER macro? Will someone sign up for doing that, or will we have n+1 ways of doing this for eternity =) https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h#ne... base/observer_list.h:114: ThisType operator++(int); Do we need to provide both prefix and postfix versions? It'd be nice to only implement the prefix one.
On 2016/09/16 21:44:17, dcheng wrote: > I haven't reviewed the entire patch yet, but can we completely remove the old > FOR_EACH_OBSERVER macro? Will someone sign up for doing that, or will we have > n+1 ways of doing this for eternity =) Yes, we can: - declare it as a deprecated - send a PSA so people can migrate - I could clean-up the leftovers > https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h > File base/observer_list.h (right): > https://codereview.chromium.org/2340583005/diff/40001/base/observer_list.h#ne... > base/observer_list.h:114: ThisType operator++(int); > Do we need to provide both prefix and postfix versions? It'd be nice to only > implement the prefix one. Ackd. Will upload the Patch Set #4.
The CQ bit was checked by loyso@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.
PTAL! My CLs are blocked on this change.
Anyone, please? Feel free to reassign if you are busy.
On 2016/09/20 00:19:00, loyso wrote: > Anyone, please? Feel free to reassign if you are busy. I'm planning on reviewing this again, but I think it would be better to not couple CLs that don't strictly require this change to this CL (as this CL looks more like a clean-up / improvement CL)
loyso@chromium.org changed reviewers: - thakis@chromium.org
Friendly ping.
I think my largest concern is whether it's important to keep the might_have_observers() short-circuiting logic. This was added in https://bugs.chromium.org/p/chromium/issues/detail?id=100121, and looks like a reasonable optimization to have. With the for-each loop, we lose that though. Have you done any measurements to verify that this has no perf impact? For example, the MessageLoop can run observers before and after dispatching a task. Is it the common case to have task observers attached now? https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne... base/observer_list.h:112: size_t checked_max_index() const { Nit: checked here makes it sound like it will CHECK or crash or something if there's an index out of bounds, but that's not really the case here. https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne... base/observer_list.h:211: return GetCurrent(); If operator* DCHECKs() this, operator-> probably should too
On 2016/09/23 07:39:02, dcheng wrote: > I think my largest concern is whether it's important to keep the > might_have_observers() short-circuiting logic. This was added in > https://bugs.chromium.org/p/chromium/issues/detail?id=100121, and looks like a > reasonable optimization to have. With the for-each loop, we lose that though. > Have you done any measurements to verify that this has no perf impact? > > For example, the MessageLoop can run observers before and after dispatching a > task. Is it the common case to have task observers attached now? Done.
https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne... base/observer_list.h:112: size_t checked_max_index() const { On 2016/09/23 07:39:01, dcheng wrote: > Nit: checked here makes it sound like it will CHECK or crash or something if > there's an index out of bounds, but that's not really the case here. Done. https://codereview.chromium.org/2340583005/diff/60001/base/observer_list.h#ne... base/observer_list.h:211: return GetCurrent(); On 2016/09/23 07:39:01, dcheng wrote: > If operator* DCHECKs() this, operator-> probably should too Done.
The CQ bit was checked by loyso@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
In progress. MSVC is special (as usual).
The CQ bit was checked by loyso@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...
All done. PTAL!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dcheng@, ping?
dcheng@, friendly ping again. It's ready to be landed.
Also, which benchmarking these things is notoriously hard, I think it'd be worth trying to cook up some benchmarks just to make sure there's not much of an impact. (One example of how to write a perftest can be seen in https://codereview.chromium.org/1874403003/) https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:88: class Iter { Derive from std::iterator. https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:180: : index_(), max_index_() {} Nit: write 0 explicitly here forconsistency. https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:349: if ((observer_list).might_have_observers()) { \ Can we structure the macro so we can use the new ranged-based for loop syntax here too? That might simplify the iterator definition itself, since we won't need to break out helpers to support the slightly different semantics of each iterator.
https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:88: class Iter { On 2016/10/03 07:29:18, dcheng wrote: > Derive from std::iterator. This comment conflicts with your previous comment "to delete the post fix ++ operator" (the one of InputIterator requirements). Your comment on "might_have_observers" optimization conflicts with ForwardIterator requirements. Please, clarify those ambiguities. Disclaimer: This CL is about very basic support of the C++ |for| loops. Iter isn't a C++ iterator in its strict C++ sense. https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:180: : index_(), max_index_() {} On 2016/10/03 07:29:18, dcheng wrote: > Nit: write 0 explicitly here forconsistency. Done. https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:349: if ((observer_list).might_have_observers()) { \ On 2016/10/03 07:29:18, dcheng wrote: > Can we structure the macro so we can use the new ranged-based for loop syntax > here too? That might simplify the iterator definition itself, since we won't > need to break out helpers to support the slightly different semantics of each > iterator. No, we can't. Some places use GetNext method and Iter construction explicitly, bypassing this macro.
On 2016/10/03 07:29:18, dcheng wrote: > Also, which benchmarking these things is notoriously hard, I think it'd be worth > trying to cook up some benchmarks just to make sure there's not much of an > impact. > > (One example of how to write a perftest can be seen in > https://codereview.chromium.org/1874403003/) In progress.
https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:88: class Iter { Also, please notice that ObserverList isn't a C++ container.
https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:88: class Iter { On 2016/10/04 04:12:02, loyso wrote: > Also, please notice that ObserverList isn't a C++ container. As discussed offline, this does work (e.g. https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree.h?...) but it's fine to leave it out, as it's generally easier to add than remove. https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:349: if ((observer_list).might_have_observers()) { \ On 2016/10/04 03:46:57, loyso wrote: > On 2016/10/03 07:29:18, dcheng wrote: > > Can we structure the macro so we can use the new ranged-based for loop syntax > > here too? That might simplify the iterator definition itself, since we won't > > need to break out helpers to support the slightly different semantics of each > > iterator. > No, we can't. Some places use GetNext method and Iter construction explicitly, > bypassing this macro. As discussed offline, let's switch the macro to use it and followup with incremental CLs to remove GetNext()
The CQ bit was checked by loyso@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
On 2016/10/04 23:53:40, dcheng wrote: > On 2016/10/04 04:12:02, loyso wrote: > > Also, please notice that ObserverList isn't a C++ container. > As discussed offline, this does work (e.g. > https://cs.chromium.org/chromium/src/content/browser/frame_host/frame_tree.h?...) Obviously, it doesn't. It doesn't even attempt to instantiate any template algorithm beyond begin/end calls. It compiles, of course. IMHO, saying that "FrameTree::NodeIterator is-a std::forward_iterator" and violating http://en.cppreference.com/w/cpp/concept/InputIterator http://en.cppreference.com/w/cpp/concept/ForwardIterator concepts is misleading. Moreover, since NodeRange violates http://en.cppreference.com/w/cpp/concept/Container requirements, the idea of NodeRange's iterator is unclear. > but it's fine to leave it out, as it's generally easier to add than remove. Ackd.
On 2016/10/04 23:53:40, dcheng wrote: > As discussed offline, this does work (e.g. p.s. That NodeIterator sub-classing will produce an error one day in C++1z with TS 19217:2015 adopted. The compiler will be able to verify all the requirements for the operations.
The CQ bit was checked by loyso@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/2340583005/diff/260001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:122: size_t index_; Let's add a comment to document the invariants for |index_|: Something like: When initially constructed and each time the iterator is incremented, |index_| is guaranteed to point to a non-null index if the iterator has not reached the end of the ObserverList. https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:192: index_(0), Then to guarantee the invariant, initialize this to GetNonNullIndex() https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:209: return GetNonNullCurrent() != other.GetNonNullCurrent(); Hmm. Given this observer list: { nullptr, nullptr, &foo, &bar } If iterator A has index_ == 0, and iterator B has index_ == 1, they'll compare equal. In some sense they are... and in some sense they aren't. Perhaps this check could be: return list_ == other_.list && index_ == other_.index_; https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:277: ObserverType* current = GetNonNullCurrent(); With the previously mentioned invariant in place, would it be possible to simplify this to: ObserverType* current = GetCurrent(); ++(*this); return current; https://codereview.chromium.org/2340583005/diff/260001/base/observer_list_uni... File base/observer_list_unittest.cc (right): https://codereview.chromium.org/2340583005/diff/260001/base/observer_list_uni... base/observer_list_unittest.cc:224: TEST(ObserverListTest, DesruptSelf) { Nit: Desrupt -> Disrupt
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/200001/base/observer_list.h#n... base/observer_list.h:349: if ((observer_list).might_have_observers()) { \ On 2016/10/04 23:53:39, dcheng wrote: > On 2016/10/04 03:46:57, loyso wrote: > > On 2016/10/03 07:29:18, dcheng wrote: > > > Can we structure the macro so we can use the new ranged-based for loop > syntax > > > here too? That might simplify the iterator definition itself, since we won't > > > need to break out helpers to support the slightly different semantics of > each > > > iterator. > > No, we can't. Some places use GetNext method and Iter construction explicitly, > > bypassing this macro. > > As discussed offline, let's switch the macro to use it and followup with > incremental CLs to remove GetNext() Done. https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:122: size_t index_; On 2016/10/05 07:08:24, dcheng wrote: > Let's add a comment to document the invariants for |index_|: > When initially constructed and each time the iterator is incremented, > |index_| is guaranteed to point to a non-null index if the iterator > has not reached the end of the ObserverList. What do you mean by "incremented"? This invariant isn't true for GetNext increment, for example. Many observers can be unsubscribe at one time. Also, new observers can subscribe. I'll check this tomorrow.
https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:277: ObserverType* current = GetNonNullCurrent(); On 2016/10/05 07:08:24, dcheng wrote: > With the previously mentioned invariant in place, would it be possible to > simplify this to: > > ObserverType* current = GetCurrent(); > ++(*this); > return current; You're right, this has to be tweaked a bit. But this can also be written: // Make sure index_ is pointing at a non-null element, if the // current index_ has been nulled out. index_ = GetNonNullIndex(); ObserverType* current = GetCurrent(); ++(*this); return current; And we won't need GetNonNullCurrent(). I'm OK with slight inaccuracy (or explicitly calling out GetNext()) in the invariant documentation previously mentioned.
The CQ bit was checked by loyso@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.
The CQ bit was checked by loyso@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...
PTAL! https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:122: size_t index_; On 2016/10/05 07:08:24, dcheng wrote: > Let's add a comment to document the invariants for |index_|: > > Something like: > > When initially constructed and each time the iterator is incremented, > |index_| is guaranteed to point to a non-null index if the iterator > has not reached the end of the ObserverList. Done. https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:192: index_(0), On 2016/10/05 07:08:23, dcheng wrote: > Then to guarantee the invariant, initialize this to GetNonNullIndex() Done. https://codereview.chromium.org/2340583005/diff/260001/base/observer_list.h#n... base/observer_list.h:209: return GetNonNullCurrent() != other.GetNonNullCurrent(); On 2016/10/05 07:08:23, dcheng wrote: > Hmm. > > Given this observer list: > > { nullptr, nullptr, &foo, &bar } > > If iterator A has index_ == 0, and iterator B has index_ == 1, they'll compare > equal. In some sense they are... and in some sense they aren't. > > Perhaps this check could be: > > return list_ == other_.list && index_ == other_.index_; Done. https://codereview.chromium.org/2340583005/diff/260001/base/observer_list_uni... File base/observer_list_unittest.cc (right): https://codereview.chromium.org/2340583005/diff/260001/base/observer_list_uni... base/observer_list_unittest.cc:224: TEST(ObserverListTest, DesruptSelf) { On 2016/10/05 07:08:24, dcheng wrote: > Nit: Desrupt -> Disrupt Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
WDYT? https://codereview.chromium.org/2340583005/diff/320001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/2340583005/diff/320001/base/observer_list.h#n... base/observer_list.h:217: if (is_end() && other.is_end()) Good catch on the is_end() thing! I think with one further tweak, we can make remove the need to explicitly check is_end(). https://codereview.chromium.org/2340583005/diff/320001/base/observer_list.h#n... base/observer_list.h:271: size_t index = index_; Let's be a little clever here to remove the need for is_end(). Since GetNonNullIndex() is always assigned to |index_|, we can just directly mutate |index_|. If we directly mutate |index_|, we can also reset the iterator state to match the default constructed iterator when we reach the end by restructuring the loop slightly: size_t max_index = clamped_max_index(); do { if (list_observers_[index]) return; } while (++index < max_index); // No more entries, set internal state to the end. list_ = nullptr; index_ = 0; (We'd probably also want to rename this to EnsureValidState() or something, since this does return a null index when iteration reaches the end of the list) Then operator== can be the simple comparison.
The CQ bit was checked by loyso@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...
On 2016/10/07 05:43:17, dcheng wrote: > WDYT? That loss of information doesn't work for GetNext and GetNext-like manual iteration. An extra observer can be added, so the list becomes non-empty again. See AddObserverInTheLastObserve unit test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. I'm hoping we can simplify the internal implementation down the road (the end iterator makes things more tricky), but I'll settle for simplifying the external interface for now.
The CQ bit was checked by loyso@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/2340583005/#ps360001 (title: "Remove excessive mutable.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Base ObserverList: Add basic support for standard C++ iterators. Also add a support for const_iterator. The range-based for loop support follows for free. See the use case in the dependent CL. BUG=634916 ========== to ========== Base ObserverList: Add basic support for standard C++ iterators. Also add a support for const_iterator. The range-based for loop support follows for free. See the use case in the dependent CL. BUG=634916 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Base ObserverList: Add basic support for standard C++ iterators. Also add a support for const_iterator. The range-based for loop support follows for free. See the use case in the dependent CL. BUG=634916 ========== to ========== Base ObserverList: Add basic support for standard C++ iterators. Also add a support for const_iterator. The range-based for loop support follows for free. See the use case in the dependent CL. BUG=634916 Committed: https://crrev.com/29025b60361ec76cf02bab720703c66ae8047be9 Cr-Commit-Position: refs/heads/master@{#424381} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/29025b60361ec76cf02bab720703c66ae8047be9 Cr-Commit-Position: refs/heads/master@{#424381} |