[PM] Migrate MaxVoteAggregator to VotingChannelWrapper.

This class was also slightly refactored to get rid of the
logic in VoteData.

Bug: 971272
Change-Id: If10c30d44371ebb00df5507e78dc4dcbd7a165e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533141
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828267}

components/performance_manager/execution_context_priority/max_vote_aggregator.cc

@@ -20,10 +20,7 @@ VotingChannel MaxVoteAggregator::GetVotingChannel() {
 }
 
 void MaxVoteAggregator::SetUpstreamVotingChannel(VotingChannel&& channel) {
-  DCHECK(channel.IsValid());
-  DCHECK(vote_data_map_.empty());
-  DCHECK(!channel_.IsValid());
-  channel_ = std::move(channel);
+  channel_.SetVotingChannel(std::move(channel));
 }
 
 void MaxVoteAggregator::OnVoteSubmitted(
@@ -32,35 +29,75 @@ void MaxVoteAggregator::OnVoteSubmitted(
     const Vote& vote) {
   DCHECK(channel_.IsValid());
 
-  // Add the new vote.
+  // Create the VoteData for this execution context, if necessary.
   VoteData& vote_data = vote_data_map_[execution_context];
-  if (vote_data.AddVote(voter_id, vote, next_vote_id_++))
-    vote_data.UpstreamVote(execution_context, &channel_);
+
+  // Remember the previous top vote before adding the new vote. There could be
+  // none if this is the first vote submitted for |execution_context|.
+  base::Optional<Vote> old_top_vote;
+  if (!vote_data.IsEmpty())
+    old_top_vote = vote_data.GetTopVote();
+
+  vote_data.AddVote(voter_id, vote, next_vote_id_++);
+
+  // If there was no previous top vote, the vote must be submitted.
+  if (!old_top_vote) {
+    channel_.SubmitVote(execution_context, vote);
+    return;
+  }
+
+  // Since there is a previous top vote, it must be modified if the top vote
+  // changed.
+  const Vote new_top_vote = vote_data.GetTopVote();
+  if (old_top_vote.value() != new_top_vote)
+    channel_.ChangeVote(execution_context, new_top_vote);
 }
 
 void MaxVoteAggregator::OnVoteChanged(VoterId voter_id,
                                       const ExecutionContext* execution_context,
                                       const Vote& new_vote) {
+  // The VoteData for this execution context is guaranteed to exist.
   VoteData& vote_data = GetVoteData(execution_context)->second;
 
-  if (vote_data.UpdateVote(voter_id, new_vote))
-    vote_data.UpstreamVote(execution_context, &channel_);
+  // Remember the previous top vote before updating the vote for this
+  // |voter_id|.
+  const Vote old_top_vote = vote_data.GetTopVote();
+
+  vote_data.UpdateVote(voter_id, new_vote);
+
+  // If the top vote changed, the upstream vote must also be changed.
+  const Vote new_top_vote = vote_data.GetTopVote();
+  if (old_top_vote != new_top_vote)
+    channel_.ChangeVote(execution_context, new_top_vote);
 }
 
 void MaxVoteAggregator::OnVoteInvalidated(
     VoterId voter_id,
     const ExecutionContext* execution_context) {
+  // The VoteData for this execution context is guaranteed to exist.
   auto it = GetVoteData(execution_context);
   VoteData& vote_data = it->second;
 
-  // Remove the vote, and upstream if necessary.
-  if (vote_data.RemoveVote(voter_id))
-    vote_data.UpstreamVote(execution_context, &channel_);
+  // Remember the previous top vote before removing the vote for this
+  // |voter_id|.
+  const Vote old_top_vote = vote_data.GetTopVote();
+
+  vote_data.RemoveVote(voter_id);
+
+  // In case the last vote for |execution_context| was invalidated, the upstream
+  // vote must also be invalidated.
+  if (vote_data.IsEmpty()) {
+    channel_.InvalidateVote(execution_context);
 
-  // If all the votes for this execution context have disappeared then remove
-  // the entry entirely. This will automatically cancel our upstream vote.
-  if (vote_data.IsEmpty())
+    // Clean up the VoteData for |execution_context| since it is empty.
     vote_data_map_.erase(it);
+    return;
+  }
+
+  // If the top vote changed, the upstream vote must also be changed.
+  const Vote new_top_vote = vote_data.GetTopVote();
+  if (old_top_vote != new_top_vote)
+    channel_.ChangeVote(execution_context, new_top_vote);
 }
 
 MaxVoteAggregator::StampedVote::StampedVote() = default;
@@ -85,33 +122,17 @@ MaxVoteAggregator::VoteData& MaxVoteAggregator::VoteData::operator=(
 
 MaxVoteAggregator::VoteData::~VoteData() = default;
 
-bool MaxVoteAggregator::VoteData::AddVote(VoterId voter_id,
+void MaxVoteAggregator::VoteData::AddVote(VoterId voter_id,
                                           const Vote& vote,
                                           uint32_t vote_id) {
-  // Remember the upstream vote as it may change. There could be none.
-  base::Optional<Vote> old_root;
-  if (!votes_.empty())
-    old_root = votes_.top().vote();
-
   auto it = votes_.emplace(vote, vote_id);
 
   bool inserted = heap_handles_.emplace(voter_id, it->handle()).second;
   DCHECK(inserted);
-
-  // There was no previous root. This vote must be upstreamed.
-  if (!old_root)
-    return true;
-
-  // The vote always needs to be upstreamed if the root vote changed.
-  const Vote& new_root = votes_.top().vote();
-  return old_root.value() != new_root;
 }
 
-bool MaxVoteAggregator::VoteData::UpdateVote(VoterId voter_id,
+void MaxVoteAggregator::VoteData::UpdateVote(VoterId voter_id,
                                              const Vote& new_vote) {
-  // Remember the upstream vote as it may change.
-  const Vote old_root = votes_.top().vote();
-
   auto it = heap_handles_.find(voter_id);
   DCHECK(it != heap_handles_.end());
   base::HeapHandle* heap_handle = it->second;
@@ -119,44 +140,19 @@ bool MaxVoteAggregator::VoteData::UpdateVote(VoterId voter_id,
   votes_.Modify(*heap_handle, [&new_vote](StampedVote& element) {
     element.SetVote(new_vote);
   });
-
-  // The vote always needs to be upstreamed if the root vote changed.
-  const Vote& new_root = votes_.top().vote();
-  return old_root != new_root;
 }
 
-bool MaxVoteAggregator::VoteData::RemoveVote(VoterId voter_id) {
-  // Remember the upstream vote as it may change.
-  const Vote old_root = votes_.top().vote();
-
+void MaxVoteAggregator::VoteData::RemoveVote(VoterId voter_id) {
   auto it = heap_handles_.find(voter_id);
   DCHECK(it != heap_handles_.end());
   base::HeapHandle* heap_handle = it->second;
   heap_handles_.erase(it);
 
   votes_.erase(*heap_handle);
-
-  // If |votes_| is now empty, the upstream vote needs to be invalidated instead
-  // of upstreaming a new vote.
-  if (votes_.empty())
-    return false;
-
-  const Vote& new_root = votes_.top().vote();
-  return old_root != new_root;
 }
 
-void MaxVoteAggregator::VoteData::UpstreamVote(
-    const ExecutionContext* execution_context,
-    VotingChannel* channel) {
-  DCHECK(!votes_.empty());
-  const Vote& vote = votes_.top().vote();
-
-  // Change our existing vote, or create a new one as necessary.
-  if (receipt_.HasVote()) {
-    receipt_.ChangeVote(vote.value(), vote.reason());
-  } else {
-    receipt_ = channel->SubmitVote(execution_context, vote);
-  }
+const Vote& MaxVoteAggregator::VoteData::GetTopVote() const {
+  return votes_.top().vote();
 }
 
 }  // namespace execution_context_priority

components/performance_manager/public/execution_context_priority/execution_context_priority.h

@@ -63,6 +63,7 @@ using VoteConsumer = voting::VoteConsumer<Vote>;
 using AcceptedVote = voting::AcceptedVote<Vote>;
 using VoteObserver = voting::VoteObserver<Vote>;
 using VoteConsumerDefaultImpl = voting::VoteConsumerDefaultImpl<Vote>;
+using VotingChannelWrapper = voting::VotingChannelWrapper<Vote>;
 
 }  // namespace execution_context_priority
 }  // namespace performance_manager

components/performance_manager/public/execution_context_priority/max_vote_aggregator.h

@@ -76,8 +76,8 @@ class MaxVoteAggregator : public VoteObserver {
 
   // The collection of votes for a single execution context. This is move-only
   // because all of its members are move-only. Internally it houses the
-  // collection of all votes associated with a execution context as max-heap,
-  // and a receipt for the vote that has been upstreamed.
+  // collection of all votes associated with an execution context as max-heap,
+  // and a map of HeapHandles to access existing votes.
   class VoteData {
    public:
     VoteData();
@@ -87,36 +87,27 @@ class MaxVoteAggregator : public VoteObserver {
     VoteData& operator=(VoteData&& rhs);
     ~VoteData();
 
-    // Adds a vote. Returns true if a new upstream vote is needed.
-    bool AddVote(VoterId voter_id, const Vote& vote, uint32_t vote_id);
+    // Adds a vote.
+    void AddVote(VoterId voter_id, const Vote& vote, uint32_t vote_id);
 
-    // Updates the vote from its given index to a new index. Returns true if the
-    // root was disturbed and a new upstream vote is needed.
-    bool UpdateVote(VoterId voter_id, const Vote& new_vote);
+    // Updates an existing vote casted by |voter_id|.
+    void UpdateVote(VoterId voter_id, const Vote& new_vote);
 
-    // Removes the vote at the provided index. Returns true if the root was
-    // disturbed and a new upstream vote is needed.
-    bool RemoveVote(VoterId voter_id);
-
-    // Upstreams the vote for this vote data, using the given voting |channel|.
-    void UpstreamVote(const ExecutionContext* execution_context,
-                      VotingChannel* channel);
-
-    // Returns the number of votes in this structure.
-    size_t GetSize() const { return votes_.size(); }
+    // Removes an existing vote casted by |voter_id|.
+    void RemoveVote(VoterId voter_id);
 
     // Returns true if this VoteData is empty.
     bool IsEmpty() const { return votes_.empty(); }
 
+    // Returns the top vote. Invalid to call if IsEmpty() returns true.
+    const Vote& GetTopVote() const;
+
    private:
     base::IntrusiveHeap<StampedVote> votes_;
 
     // Maps each voting channel to the HeapHandle to their associated vote in
     // |votes_|.
     std::map<VoterId, base::HeapHandle*> heap_handles_;
-
-    // The receipt for the vote we've upstreamed.
-    VoteReceipt receipt_;
   };
 
   using VoteDataMap = std::map<const ExecutionContext*, VoteData>;
@@ -126,7 +117,7 @@ class MaxVoteAggregator : public VoteObserver {
   VoteDataMap::iterator GetVoteData(const ExecutionContext* execution_context);
 
   // Our channel for upstreaming our votes.
-  VotingChannel channel_;
+  VotingChannelWrapper channel_;
 
   // Provides VotingChannels to our input voters.
   VoteConsumerDefaultImpl vote_consumer_default_impl_;

components/performance_manager/public/voting/voting.h

@@ -481,6 +481,9 @@ class VotingChannelWrapper {
   // was invoked.
   void InvalidateVote(const ContextType* context);
 
+  // Returns true if the underlying VotingChannel is valid.
+  bool IsValid() const;
+
  private:
   VotingChannel<VoteImpl> voting_channel_;
 
@@ -1011,6 +1014,11 @@ void VotingChannelWrapper<VoteImpl>::InvalidateVote(
   DCHECK_EQ(removed, 1u);
 }
 
+template <class VoteImpl>
+bool VotingChannelWrapper<VoteImpl>::IsValid() const {
+  return voting_channel_.IsValid();
+}
+
 }  // namespace voting
 }  // namespace performance_manager