Global Resource Coordinator: Basic service internals

services/resource_coordinator/coordination_unit/coordination_unit_impl.cc

Index: services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
diff --git a/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc b/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
new file mode 100644
index 0000000000000000000000000000000000000000..07d3c11c7fecc5de4301ceaa93eb1cdc2c0f57dc
--- /dev/null
+++ b/services/resource_coordinator/coordination_unit/coordination_unit_impl.cc
@@ -0,0 +1,205 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "services/resource_coordinator/coordination_unit/coordination_unit_impl.h"
+
+#include "base/hash.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "services/resource_coordinator/public/cpp/coordination_unit_events.h"
+#include "services/service_manager/public/cpp/connection.h"
+
+namespace resource_coordinator {
+
+namespace {
+
+base::LazyInstance<std::unordered_map<int64_t, CoordinationUnitImpl*>>::Leaky

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

Since we have thread-safe statics in C++11, I think you can just rewrite this
as:

using CUIDMap = std::unordered_map<int64_t, CoordinationUnitImpl*>;
CUIDMap* g_cuid_map() {
  static CUIDMap* instance = new CUIDMap();
  return instance;
}

that would avoid the extra Get() below

Precedent for this:
https://cs.chromium.org/search/?q=%5C(%5C)%5C+%7B%5Cn%5C+%5C+static.*%5Cn%5C+...

[oystein (OOO til 10th of July), 2017/04/10 20:02:58]

Oh glad we can do this now; done!

+    g_cu_map = LAZY_INSTANCE_INITIALIZER;
+
+int64_t CUIDHash(const CoordinationUnitID& id) {
+  return (((uint64_t)id.type) << 32) ^ id.id;

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

(uint64_t) -> static_cast<uint64_t>.
I hate it and find it redundant, but our style guide explicitly says "Do not use
C-style casts." :-/
Also, purely documentational, but since id is a int32_t, this could just be a
s/^/|/ right?

In any case, if you just make this a proper hash operator of CoordinationUnitID,
then you can just make a 
unordered_map<CUID, CoordinationUnitImpl*>, which simplifies a bit the code
below, avoiding to hash explicitly

[oystein (OOO til 10th of July), 2017/04/10 20:02:59]

Done and done.

+}
+
+}  // namespace
+
+CoordinationUnitImpl::CoordinationUnitImpl(

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

on which thread are these CUnits built and destroyed?
By the lack of locking, it seems that the assumptions is that everything happens
on the same thread.
Maybe you could have a global threadchecker (or sequencechecker) next to the map
to make sure we don't accidentally bereak this in future (threading did bite me
lot of times in the past in memory infra, especially in the cases where it was
out of my control and up to the clients)

[oystein (OOO til 10th of July), 2017/04/10 20:02:59]

Mojo guarantees it'll all be run on the same taskrunner, but I added a
threadchecker to the ResourceCoordinatorInterface since that's client code and
could potentially be used from multiple threads.

[Primiano Tucci (use gerrit), 2017/04/11 18:04:51]

Acknowledged.

+    const CoordinationUnitID& id,
+    std::unique_ptr<service_manager::ServiceContextRef> service_ref)
+    : id_(id) {
+  id_hash_ = CUIDHash(id);
+
+  auto& cu_map = g_cu_map.Get();
+  DCHECK(cu_map.end() == cu_map.find(id_hash_));

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

really a minor perf thingy, as this is debug only, but I think that
map.count(id_hash) == 0 is slightly faster (or generally not slower than .find)
as doesn't have to build up an iterator.ù

Or eventually you could just
auto it = cu_map.insert(std::make_pair(id_hash_, this));
DCHECK(it.second /* did insert*/);

[oystein (OOO til 10th of July), 2017/04/10 20:02:57]

Done; didn't realize this but makes sense in hindsight given that unordered_map
won't have duplicate elements and could in theory just stop at the first
encountered element and return 1. Some surface Googling suggests count() may
often be implemented using find() internally, but I went with the second
suggestion regardless since it looks a little cleaner.

[Primiano Tucci (use gerrit), 2017/04/11 18:04:51]

Acknowledged.

+
+  cu_map.insert(std::make_pair(id_hash_, this));
+  service_ref_ = std::move(service_ref);
+}
+
+CoordinationUnitImpl::~CoordinationUnitImpl() {
+  g_cu_map.Get().erase(id_hash_);
+
+  for (auto* child : children_) {
+    child->RemoveParent(this);
+  }
+
+  for (auto* parent : parents_) {
+    parent->RemoveChild(this);
+  }
+}
+
+bool CoordinationUnitImpl::SelfOrParentHasFlagSet(StateFlags state) {
+  const base::Optional<bool>& state_flag = state_flags_[state];
+  if (state_flag && *state_flag)
+    return true;
+
+  for (auto* parent : parents_) {
+    if (parent->SelfOrParentHasFlagSet(state))
+      return true;
+  }
+
+  return false;
+}
+
+void CoordinationUnitImpl::RecalcPolicy() {
+  for (auto* child : children_) {
+    child->RecalcPolicy();
+  }
+
+  if (!policy_callback_) {
+    return;
+  }
+
+  bool background_priority = !SelfOrParentHasFlagSet(TAB_VISIBLE) &&
+                             !SelfOrParentHasFlagSet(AUDIO_PLAYING);
+
+  // Send the priority to the client if it's new or changed.
+  if (!current_policy_)
+    current_policy_ = mojom::Policy::New();
+  else if ((current_policy_->use_background_priority == background_priority) &&
+           !SelfOrParentHasFlagSet(TEST_STATE)) {
+    return;
+  }
+
+  mojom::PolicyPtr policy = mojom::Policy::New();
+  policy->use_background_priority = background_priority;
+  current_policy_->use_background_priority = background_priority;
+
+  policy_callback_->SetPolicy(std::move(policy));
+}
+
+void CoordinationUnitImpl::SendEvent(mojom::EventPtr event) {
+  switch (event->type) {
+    case ON_WEBCONTENTS_SHOWN:
+      state_flags_[TAB_VISIBLE] = true;
+      break;
+    case ON_WEBCONTENTS_HIDDEN:
+      state_flags_[TAB_VISIBLE] = false;
+      break;
+    case ON_PROCESS_AUDIO_STARTED:
+      state_flags_[AUDIO_PLAYING] = true;
+      break;
+    case ON_PROCESS_AUDIO_STOPPED:
+      state_flags_[AUDIO_PLAYING] = false;
+      break;
+    case TEST_EVENT:
+      state_flags_[TEST_STATE] = true;
+      break;
+    default:
+      return;
+  }
+
+  RecalcPolicy();
+}
+
+void CoordinationUnitImpl::GetID(const GetIDCallback& callback) {
+  callback.Run(id_);

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

uh? This might deserve some comment :)
why the caller doesn't just do it?

[oystein (OOO til 10th of July), 2017/04/10 20:02:59]

Added a comment in the .mojom.

It's mainly if you do things like:
* Create CU a
* Create CU b
* a.AddChild(b)

Since each CU has a separate messagepipe and these are async calls, there's no
guarantee that B will exist when then AddChild gets executed on the service
side. GetID() is basically just there to force a roundtrip over b's messagepipe
so we don't have to deal with a map of pending parent-child relationships or
something.

+}
+
+void CoordinationUnitImpl::Duplicate(mojom::CoordinationUnitRequest request) {
+  bindings_.AddBinding(this, std::move(request));
+}
+
+void CoordinationUnitImpl::AddChild(const CoordinationUnitID& child_id) {
+  if (child_id == id_)
+    return;
+
+  auto child_iter = g_cu_map.Get().find(CUIDHash(child_id));
+  if (child_iter != g_cu_map.Get().end()) {
+    CoordinationUnitImpl* child = child_iter->second;
+    if (HasParent(child) || HasChild(child))
+      return;
+
+    DCHECK(child->id_ == child_id);

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

Both DCHECK(a == b) or DCHECK_EQ(a, b) are fine, I guess just use them
consistently (you use the _EQ down below on line 155).

[oystein (OOO til 10th of July), 2017/04/10 20:02:57]

That was my first attempt at this check, but I get:
../../base/logging.h:659:3: error: no matching function for call to
'MakeCheckOpValueString'

(...)

../../base/logging.h:649:18: note: candidate function not viable: no known
conversion from 'const resource_coordinator::CoordinationUnitID' to
'std::nullptr_t' (aka 'nullptr_t') for 2nd argument

I've no idea why this needs to be convertable to nullptr_ and it felt like it'd
be a rathole trying to figure that out :).

+    DCHECK(child != this);
+
+    if (AddChild(child)) {
+      child->AddParent(this);
+    }
+  }
+}
+
+bool CoordinationUnitImpl::AddChild(CoordinationUnitImpl* child) {
+  if (children_.find(child) != children_.end())

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

just
return children_.count(child) ? false : childer_.insert(child).second ?
would save 5 lines :)

[oystein (OOO til 10th of July), 2017/04/10 20:02:59]

Done.

+    return false;
+
+  children_.insert(child);
+
+  return true;
+}
+
+void CoordinationUnitImpl::RemoveChild(CoordinationUnitImpl* child) {
+  size_t children_removed = children_.erase(child);
+  CHECK_EQ(1u, children_removed);

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

just checking: is this really intended to be a check, or typoed a D? All the
others are DCHECK?

[oystein (OOO til 10th of July), 2017/04/10 20:02:59]

Typo; fixed.

+}
+
+void CoordinationUnitImpl::AddParent(CoordinationUnitImpl* parent) {
+  DCHECK(parents_.find(parent) == parents_.end());
+  parents_.insert(parent);
+
+  RecalcPolicy();
+}
+
+void CoordinationUnitImpl::RemoveParent(CoordinationUnitImpl* parent) {
+  size_t parents_removed = parents_.erase(parent);
+  CHECK_EQ(1u, parents_removed);
+
+  RecalcPolicy();
+}
+
+bool CoordinationUnitImpl::HasParent(CoordinationUnitImpl* unit) {
+  for (auto* parent : parents_) {
+    if (parent == unit || parent->HasParent(unit))
+      return true;
+  }
+
+  return false;
+}
+
+bool CoordinationUnitImpl::HasChild(CoordinationUnitImpl* unit) {
+  for (auto* child : children_) {
+    if (child == unit || child->HasChild(unit))
+      return true;
+  }
+
+  return false;
+}
+
+void CoordinationUnitImpl::SetPolicyCallback(
+    mojom::PolicyCallbackPtr callback) {
+  callback.set_connection_error_handler(base::Bind(
+      &CoordinationUnitImpl::UnregisterPolicyCallback, base::Unretained(this)));

[Primiano Tucci (use gerrit), 2017/04/06 18:09:46]

what happens if the CUI is destroyed in the meanwhile? the dtor suggests this is
not undefinitely lived.
Should this have a weakptr instead?

[oystein (OOO til 10th of July), 2017/04/10 20:02:59]

Mojo handles the lifespan and will only call this on alive bindings, seems to be
the pattern used elsewhere too.

+
+  policy_callback_ = std::move(callback);
+
+  RecalcPolicy();
+}
+
+void CoordinationUnitImpl::UnregisterPolicyCallback() {
+  policy_callback_.reset();
+  current_policy_.reset();
+}
+
+}  // namespace resource_coordinator