You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2018/10/05 18:05:33 UTC
[mesos] 03/04: Mitigated accidental mutation of
`Resources::resources`.
This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a commit to branch 1.7.x
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 319482bc4f3dde2ad3d33e889a6edb175c85a1cd
Author: Meng Zhu <mz...@mesosphere.io>
AuthorDate: Thu Sep 20 14:03:00 2018 -0700
Mitigated accidental mutation of `Resources::resources`.
Due to the copy-on-write optimization (MESOS-6765), one needs to
check the `use_count` of `Resource_` before mutating. Currently,
there is no mechanism to enforce this. As a short-term mitigation
measure, we rename `resources` to
`resourcesNoMutationWithoutExclusiveOwnership` and typedef its item
type to `Resource_UnSafe`
to alert people about obtaining an exclusive ownership before mutating
the `Resource_` objects.
---
include/mesos/resources.hpp | 61 +++++++++++++------
include/mesos/v1/resources.hpp | 61 +++++++++++++------
src/common/resources.cpp | 133 +++++++++++++++++++++++++++++------------
src/v1/resources.cpp | 125 +++++++++++++++++++++++++++-----------
4 files changed, 267 insertions(+), 113 deletions(-)
diff --git a/include/mesos/resources.hpp b/include/mesos/resources.hpp
index 914b7be..36ccf0e 100644
--- a/include/mesos/resources.hpp
+++ b/include/mesos/resources.hpp
@@ -168,6 +168,11 @@ private:
};
public:
+ // We rename the type here to alert people about the fact that with
+ // `shared_ptr`, no mutation should be made without obtaining exclusive
+ // ownership. See `resourcesNoMutationWithoutExclusiveOwnership`.
+ using Resource_Unsafe = std::shared_ptr<Resource_>;
+
/**
* Returns a Resource with the given name, value, and role.
*
@@ -407,7 +412,8 @@ public:
Resources& operator=(const Resources& that)
{
if (this != &that) {
- resources = that.resources;
+ resourcesNoMutationWithoutExclusiveOwnership =
+ that.resourcesNoMutationWithoutExclusiveOwnership;
}
return *this;
}
@@ -415,14 +421,21 @@ public:
Resources& operator=(Resources&& that)
{
if (this != &that) {
- resources = std::move(that.resources);
+ resourcesNoMutationWithoutExclusiveOwnership =
+ std::move(that.resourcesNoMutationWithoutExclusiveOwnership);
}
return *this;
}
- bool empty() const { return resources.size() == 0; }
+ bool empty() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.size() == 0;
+ }
- size_t size() const { return resources.size(); }
+ size_t size() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.size();
+ }
// Checks if this Resources is a superset of the given Resources.
bool contains(const Resources& that) const;
@@ -589,31 +602,38 @@ public:
// We use `boost::indirect_iterator` to expose `Resource` (implicitly
// converted from `Resource_`) iteration, while actually storing
- // `shared_ptr<Resource_>`.
+ // `Resource_Unsafe`.
//
// NOTE: Non-const `begin()` and `end()` intentionally return const
// iterators to prevent mutable access to the `Resource` objects.
typedef boost::indirect_iterator<
- std::vector<std::shared_ptr<Resource_>>::const_iterator>
+ std::vector<Resource_Unsafe>::const_iterator>
const_iterator;
const_iterator begin()
{
- return static_cast<const std::vector<std::shared_ptr<Resource_>>&>(
- resources)
+ return static_cast<const std::vector<Resource_Unsafe>&>(
+ resourcesNoMutationWithoutExclusiveOwnership)
.begin();
}
const_iterator end()
{
- return static_cast<const std::vector<std::shared_ptr<Resource_>>&>(
- resources)
+ return static_cast<const std::vector<Resource_Unsafe>&>(
+ resourcesNoMutationWithoutExclusiveOwnership)
.end();
}
- const_iterator begin() const { return resources.begin(); }
- const_iterator end() const { return resources.end(); }
+ const_iterator begin() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.begin();
+ }
+
+ const_iterator end() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.end();
+ }
// Using this operator makes it easy to copy a resources object into
// a protocol buffer field.
@@ -679,7 +699,7 @@ private:
void add(Resource_&& r);
// TODO(mzhu): Add move support.
- void add(const std::shared_ptr<Resource_>& that);
+ void add(const Resource_Unsafe& that);
void subtract(const Resource_& r);
@@ -701,15 +721,18 @@ private:
// (b) If there's more than a single reference to the
// resource object, we copy first, then mutate the copy.
//
- // TODO(mzhu): When mutating the resource objects, we need to manually
- // conduct the copy-on-write check as described in (2). This is
- // brittle. Explore more robust designs such as introducing a customized
- // copy-on-write abstraction that hides direct setters and only allow
- // mutations in a controlled fashion.
+ // We name the `vector` field `resourcesNoMutationWithoutExclusiveOwnership`
+ // and typedef its item type to `Resource_Unsafe` to alert people
+ // regarding (2).
+ //
+ // TODO(mzhu): While naming the vector and its item type may help, this is
+ // still brittle and certainly not ideal. Explore more robust designs such as
+ // introducing a customized copy-on-write abstraction that hides direct
+ // setters and only allow mutations in a controlled fashion.
//
// TODO(mzhu): Consider using `boost::intrusive_ptr` for
// possibly better performance.
- std::vector<std::shared_ptr<Resource_>> resources;
+ std::vector<Resource_Unsafe> resourcesNoMutationWithoutExclusiveOwnership;
};
diff --git a/include/mesos/v1/resources.hpp b/include/mesos/v1/resources.hpp
index 07d6092..1a9ea44 100644
--- a/include/mesos/v1/resources.hpp
+++ b/include/mesos/v1/resources.hpp
@@ -167,6 +167,11 @@ private:
};
public:
+ // We rename the type here to alert people about the fact that with
+ // `shared_ptr`, no mutation should be made without obtaining exclusive
+ // ownership. See `resourcesNoMutationWithoutExclusiveOwnership`.
+ using Resource_Unsafe = std::shared_ptr<Resource_>;
+
/**
* Returns a Resource with the given name, value, and role.
*
@@ -406,7 +411,8 @@ public:
Resources& operator=(const Resources& that)
{
if (this != &that) {
- resources = that.resources;
+ resourcesNoMutationWithoutExclusiveOwnership =
+ that.resourcesNoMutationWithoutExclusiveOwnership;
}
return *this;
}
@@ -414,14 +420,21 @@ public:
Resources& operator=(Resources&& that)
{
if (this != &that) {
- resources = std::move(that.resources);
+ resourcesNoMutationWithoutExclusiveOwnership =
+ std::move(that.resourcesNoMutationWithoutExclusiveOwnership);
}
return *this;
}
- bool empty() const { return resources.size() == 0; }
+ bool empty() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.size() == 0;
+ }
- size_t size() const { return resources.size(); }
+ size_t size() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.size();
+ }
// Checks if this Resources is a superset of the given Resources.
bool contains(const Resources& that) const;
@@ -583,31 +596,38 @@ public:
// We use `boost::indirect_iterator` to expose `Resource` (implicitly
// converted from `Resource_`) iteration, while actually storing
- // `shared_ptr<Resource_>`.
+ // `Resource_Unsafe`.
//
// NOTE: Non-const `begin()` and `end()` intentionally return const
// iterators to prevent mutable access to the `Resource` objects.
typedef boost::indirect_iterator<
- std::vector<std::shared_ptr<Resource_>>::const_iterator>
+ std::vector<Resource_Unsafe>::const_iterator>
const_iterator;
const_iterator begin()
{
- return static_cast<const std::vector<std::shared_ptr<Resource_>>&>(
- resources)
+ return static_cast<const std::vector<Resource_Unsafe>&>(
+ resourcesNoMutationWithoutExclusiveOwnership)
.begin();
}
const_iterator end()
{
- return static_cast<const std::vector<std::shared_ptr<Resource_>>&>(
- resources)
+ return static_cast<const std::vector<Resource_Unsafe>&>(
+ resourcesNoMutationWithoutExclusiveOwnership)
.end();
}
- const_iterator begin() const { return resources.begin(); }
- const_iterator end() const { return resources.end(); }
+ const_iterator begin() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.begin();
+ }
+
+ const_iterator end() const
+ {
+ return resourcesNoMutationWithoutExclusiveOwnership.end();
+ }
// Using this operator makes it easy to copy a resources object into
// a protocol buffer field.
@@ -673,7 +693,7 @@ private:
void add(Resource_&& r);
// TODO(mzhu): Add move support.
- void add(const std::shared_ptr<Resource_>& that);
+ void add(const Resource_Unsafe& that);
void subtract(const Resource_& r);
@@ -695,15 +715,18 @@ private:
// (b) If there's more than a single reference to the
// resource object, we copy first, then mutate the copy.
//
- // TODO(mzhu): When mutating the resource objects, we need to manually
- // conduct the copy-on-write check as described in (2). This is
- // brittle. Explore more robust designs such as introducing a customized
- // copy-on-write abstraction that hides direct setters and only allow
- // mutations in a controlled fashion.
+ // We name the `vector` field `resourcesNoMutationWithoutExclusiveOwnership`
+ // and typedef its item type to `Resource_Unsafe` to alert people
+ // regarding (2).
+ //
+ // TODO(mzhu): While naming the vector and its item type may help, this is
+ // still brittle and certainly not ideal. Explore more robust designs such as
+ // introducing a customized copy-on-write abstraction that hides direct
+ // setters and only allow mutations in a controlled fashion.
//
// TODO(mzhu): Consider using `boost::intrusive_ptr` for
// possibly better performance.
- std::vector<std::shared_ptr<Resource_>> resources;
+ std::vector<Resource_Unsafe> resourcesNoMutationWithoutExclusiveOwnership;
};
diff --git a/src/common/resources.cpp b/src/common/resources.cpp
index 4719ea5..f7b265d 100644
--- a/src/common/resources.cpp
+++ b/src/common/resources.cpp
@@ -1428,7 +1428,7 @@ Resources::Resources(Resource&& resource)
Resources::Resources(const vector<Resource>& _resources)
{
- resources.reserve(_resources.size());
+ resourcesNoMutationWithoutExclusiveOwnership.reserve(_resources.size());
foreach (const Resource& resource, _resources) {
// NOTE: Invalid and zero Resource objects will be ignored.
*this += resource;
@@ -1438,7 +1438,7 @@ Resources::Resources(const vector<Resource>& _resources)
Resources::Resources(vector<Resource>&& _resources)
{
- resources.reserve(_resources.size());
+ resourcesNoMutationWithoutExclusiveOwnership.reserve(_resources.size());
foreach (Resource& resource, _resources) {
// NOTE: Invalid and zero Resource objects will be ignored.
*this += std::move(resource);
@@ -1448,7 +1448,7 @@ Resources::Resources(vector<Resource>&& _resources)
Resources::Resources(const RepeatedPtrField<Resource>& _resources)
{
- resources.reserve(_resources.size());
+ resourcesNoMutationWithoutExclusiveOwnership.reserve(_resources.size());
foreach (const Resource& resource, _resources) {
// NOTE: Invalid and zero Resource objects will be ignored.
*this += resource;
@@ -1458,7 +1458,7 @@ Resources::Resources(const RepeatedPtrField<Resource>& _resources)
Resources::Resources(RepeatedPtrField<Resource>&& _resources)
{
- resources.reserve(_resources.size());
+ resourcesNoMutationWithoutExclusiveOwnership.reserve(_resources.size());
foreach (Resource& resource, _resources) {
// NOTE: Invalid and zero Resource objects will be ignored.
*this += std::move(resource);
@@ -1470,7 +1470,9 @@ bool Resources::contains(const Resources& that) const
{
Resources remaining = *this;
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
// NOTE: We use _contains because Resources only contain valid
// Resource objects, and we don't want the performance hit of the
// validity check.
@@ -1498,7 +1500,9 @@ bool Resources::contains(const Resource& that) const
size_t Resources::count(const Resource& that) const
{
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource == that) {
// Return 1 for non-shared resources because non-shared
// Resource objects in Resources are unique.
@@ -1512,7 +1516,9 @@ size_t Resources::count(const Resource& that) const
void Resources::allocate(const string& role)
{
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
resource_ = make_shared<Resource_>(*resource_);
@@ -1524,7 +1530,9 @@ void Resources::allocate(const string& role)
void Resources::unallocate()
{
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.has_allocation_info()) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -1540,7 +1548,9 @@ Resources Resources::filter(
const lambda::function<bool(const Resource&)>& predicate) const
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (predicate(resource_->resource)) {
// TODO(mzhu): `add` is O(n), explore whether we can simply
// do `push_back` by assuming resources are already combined.
@@ -1555,7 +1565,9 @@ hashmap<string, Resources> Resources::reservations() const
{
hashmap<string, Resources> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (isReserved(resource_->resource)) {
result[reservationRole(resource_->resource)].add(resource_);
}
@@ -1619,7 +1631,9 @@ hashmap<string, Resources> Resources::allocations() const
{
hashmap<string, Resources> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
// We require that this is called only when
// the resources are allocated.
CHECK(resource_->resource.has_allocation_info());
@@ -1638,7 +1652,9 @@ Resources Resources::pushReservation(
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
Resource_ r_ = *resource_;
r_.resource.add_reservations()->CopyFrom(reservation);
CHECK_NONE(Resources::validate(r_.resource));
@@ -1653,7 +1669,9 @@ Resources Resources::popReservation() const
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
CHECK_GT(resource_->resource.reservations_size(), 0);
Resource_ r_ = *resource_;
r_.resource.mutable_reservations()->RemoveLast();
@@ -1668,7 +1686,9 @@ Resources Resources::toUnreserved() const
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (isReserved(resource_->resource)) {
Resource_ r_ = *resource_;
r_.resource.clear_reservations();
@@ -1686,7 +1706,9 @@ Resources Resources::createStrippedScalarQuantity() const
{
Resources stripped;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.type() == Value::SCALAR) {
Resource scalar;
@@ -1762,7 +1784,9 @@ Option<Value::Scalar> Resources::get(const string& name) const
Value::Scalar total;
bool found = false;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.name() == name &&
resource_->resource.type() == Value::SCALAR) {
total += resource_->resource.scalar();
@@ -1784,7 +1808,9 @@ Option<Value::Set> Resources::get(const string& name) const
Value::Set total;
bool found = false;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.name() == name &&
resource_->resource.type() == Value::SET) {
total += resource_->resource.set();
@@ -1806,7 +1832,9 @@ Option<Value::Ranges> Resources::get(const string& name) const
Value::Ranges total;
bool found = false;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.name() == name &&
resource_->resource.type() == Value::RANGES) {
total += resource_->resource.ranges();
@@ -1841,7 +1869,9 @@ Resources Resources::scalars() const
set<string> Resources::names() const
{
set<string> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
result.insert(resource_->resource.name());
}
@@ -1852,7 +1882,9 @@ set<string> Resources::names() const
map<string, Value_Type> Resources::types() const
{
map<string, Value_Type> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
result[resource_->resource.name()] = resource_->resource.type();
}
@@ -1928,7 +1960,9 @@ Option<Value::Ranges> Resources::ephemeral_ports() const
Option<Resource> Resources::match(const Resource& that) const
{
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (compareResourceMetadata(resource_->resource, that)) {
return resource_->resource;
}
@@ -1943,7 +1977,9 @@ Option<Resource> Resources::match(const Resource& that) const
bool Resources::_contains(const Resource_& that) const
{
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->contains(that)) {
return true;
}
@@ -1972,8 +2008,8 @@ Option<Resources> Resources::find(const Resource& target) const
foreach (const auto& predicate, predicates) {
foreach (
- const shared_ptr<Resource_>& resource_,
- total.filter(predicate).resources) {
+ const Resource_Unsafe& resource_,
+ total.filter(predicate).resourcesNoMutationWithoutExclusiveOwnership) {
// Need to `toUnreserved` to ignore the roles in contains().
Resources unreserved;
unreserved.add(resource_);
@@ -2009,7 +2045,9 @@ Option<Resources> Resources::find(const Resource& target) const
Resources::operator RepeatedPtrField<Resource>() const
{
RepeatedPtrField<Resource> all;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
all.Add()->CopyFrom(resource_->resource);
}
@@ -2100,7 +2138,9 @@ void Resources::add(const Resource_& that)
}
bool found = false;
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (internal::addable(resource_->resource, that.resource)) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -2115,7 +2155,8 @@ void Resources::add(const Resource_& that)
// Cannot be combined with any existing Resource object.
if (!found) {
- resources.push_back(make_shared<Resource_>(that));
+ resourcesNoMutationWithoutExclusiveOwnership.push_back(
+ make_shared<Resource_>(that));
}
}
@@ -2127,7 +2168,9 @@ void Resources::add(Resource_&& that)
}
bool found = false;
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (internal::addable(resource_->resource, that.resource)) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -2144,19 +2187,22 @@ void Resources::add(Resource_&& that)
// Cannot be combined with any existing Resource object.
if (!found) {
- resources.push_back(make_shared<Resource_>(std::move(that)));
+ resourcesNoMutationWithoutExclusiveOwnership.push_back(
+ make_shared<Resource_>(std::move(that)));
}
}
-void Resources::add(const shared_ptr<Resource_>& that)
+void Resources::add(const Resource_Unsafe& that)
{
if (that->isEmpty()) {
return;
}
bool found = false;
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (internal::addable(resource_->resource, that->resource)) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -2171,7 +2217,7 @@ void Resources::add(const shared_ptr<Resource_>& that)
// Cannot be combined with any existing Resource object.
if (!found) {
- resources.push_back(that);
+ resourcesNoMutationWithoutExclusiveOwnership.push_back(that);
}
}
@@ -2214,7 +2260,9 @@ Resources& Resources::operator+=(Resource&& that)
Resources& Resources::operator+=(const Resources& that)
{
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
add(resource_);
}
@@ -2224,7 +2272,9 @@ Resources& Resources::operator+=(const Resources& that)
Resources& Resources::operator+=(Resources&& that)
{
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
add(std::move(resource_));
}
@@ -2254,8 +2304,10 @@ void Resources::subtract(const Resource_& that)
return;
}
- for (size_t i = 0; i < resources.size(); i++) {
- shared_ptr<Resource_>& resource_ = resources[i];
+ for (size_t i = 0; i < resourcesNoMutationWithoutExclusiveOwnership.size();
+ i++) {
+ Resource_Unsafe& resource_ =
+ resourcesNoMutationWithoutExclusiveOwnership[i];
if (internal::subtractable(resource_->resource, that)) {
// Copy-on-write (if more than 1 reference).
@@ -2283,8 +2335,9 @@ void Resources::subtract(const Resource_& that)
// As `resources` is not ordered, and erasing an element
// from the middle is expensive, we swap with the last element
// and then shrink the vector by one.
- resources[i] = resources.back();
- resources.pop_back();
+ resourcesNoMutationWithoutExclusiveOwnership[i] =
+ resourcesNoMutationWithoutExclusiveOwnership.back();
+ resourcesNoMutationWithoutExclusiveOwnership.pop_back();
}
break;
@@ -2313,7 +2366,9 @@ Resources& Resources::operator-=(const Resource& that)
Resources& Resources::operator-=(const Resources& that)
{
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
subtract(*resource_);
}
diff --git a/src/v1/resources.cpp b/src/v1/resources.cpp
index 49f3e0f..8bd9f33 100644
--- a/src/v1/resources.cpp
+++ b/src/v1/resources.cpp
@@ -1457,7 +1457,7 @@ Resources::Resources(const vector<Resource>& _resources)
Resources::Resources(vector<Resource>&& _resources)
{
- resources.reserve(_resources.size());
+ resourcesNoMutationWithoutExclusiveOwnership.reserve(_resources.size());
foreach (Resource& resource, _resources) {
// NOTE: Invalid and zero Resource objects will be ignored.
*this += std::move(resource);
@@ -1476,7 +1476,7 @@ Resources::Resources(const RepeatedPtrField<Resource>& _resources)
Resources::Resources(RepeatedPtrField<Resource>&& _resources)
{
- resources.reserve(_resources.size());
+ resourcesNoMutationWithoutExclusiveOwnership.reserve(_resources.size());
foreach (Resource& resource, _resources) {
// NOTE: Invalid and zero Resource objects will be ignored.
*this += std::move(resource);
@@ -1488,7 +1488,9 @@ bool Resources::contains(const Resources& that) const
{
Resources remaining = *this;
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
// NOTE: We use _contains because Resources only contain valid
// Resource objects, and we don't want the performance hit of the
// validity check.
@@ -1516,7 +1518,9 @@ bool Resources::contains(const Resource& that) const
size_t Resources::count(const Resource& that) const
{
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource == that) {
// Return 1 for non-shared resources because non-shared
// Resource objects in Resources are unique.
@@ -1530,7 +1534,9 @@ size_t Resources::count(const Resource& that) const
void Resources::allocate(const string& role)
{
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
resource_ = make_shared<Resource_>(*resource_);
@@ -1542,7 +1548,9 @@ void Resources::allocate(const string& role)
void Resources::unallocate()
{
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.has_allocation_info()) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -1558,7 +1566,9 @@ Resources Resources::filter(
const lambda::function<bool(const Resource&)>& predicate) const
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (predicate(resource_->resource)) {
// TODO(mzhu): `add` is O(n), investigate whether we can simply
// do `push_back` by assuming resources are already combined.
@@ -1573,7 +1583,9 @@ hashmap<string, Resources> Resources::reservations() const
{
hashmap<string, Resources> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (isReserved(resource_->resource)) {
result[reservationRole(resource_->resource)].add(resource_);
}
@@ -1637,7 +1649,9 @@ hashmap<string, Resources> Resources::allocations() const
{
hashmap<string, Resources> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
// We require that this is called only when
// the resources are allocated.
CHECK(resource_->resource.has_allocation_info());
@@ -1654,7 +1668,9 @@ Resources Resources::pushReservation(
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
Resource_ r_ = *resource_;
r_.resource.add_reservations()->CopyFrom(reservation);
CHECK_NONE(Resources::validate(r_.resource));
@@ -1669,7 +1685,9 @@ Resources Resources::popReservation() const
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
CHECK_GT(resource_->resource.reservations_size(), 0);
Resource_ r_ = *resource_;
r_.resource.mutable_reservations()->RemoveLast();
@@ -1684,7 +1702,9 @@ Resources Resources::toUnreserved() const
{
Resources result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (isReserved(resource_->resource)) {
Resource_ r_ = *resource_;
r_.resource.clear_reservations();
@@ -1702,7 +1722,9 @@ Resources Resources::createStrippedScalarQuantity() const
{
Resources stripped;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.type() == Value::SCALAR) {
Resource scalar;
@@ -1777,7 +1799,9 @@ Option<Value::Scalar> Resources::get(const string& name) const
Value::Scalar total;
bool found = false;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.name() == name &&
resource_->resource.type() == Value::SCALAR) {
total += resource_->resource.scalar();
@@ -1799,7 +1823,9 @@ Option<Value::Set> Resources::get(const string& name) const
Value::Set total;
bool found = false;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.name() == name &&
resource_->resource.type() == Value::SET) {
total += resource_->resource.set();
@@ -1821,7 +1847,9 @@ Option<Value::Ranges> Resources::get(const string& name) const
Value::Ranges total;
bool found = false;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->resource.name() == name &&
resource_->resource.type() == Value::RANGES) {
total += resource_->resource.ranges();
@@ -1856,7 +1884,9 @@ Resources Resources::scalars() const
set<string> Resources::names() const
{
set<string> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
result.insert(resource_->resource.name());
}
@@ -1867,7 +1897,9 @@ set<string> Resources::names() const
map<string, Value_Type> Resources::types() const
{
map<string, Value_Type> result;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
result[resource_->resource.name()] = resource_->resource.type();
}
@@ -1947,7 +1979,9 @@ Option<Value::Ranges> Resources::ephemeral_ports() const
bool Resources::_contains(const Resource_& that) const
{
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (resource_->contains(that)) {
return true;
}
@@ -1976,8 +2010,8 @@ Option<Resources> Resources::find(const Resource& target) const
foreach (const auto& predicate, predicates) {
foreach (
- const shared_ptr<Resource_>& resource_,
- total.filter(predicate).resources) {
+ const Resource_Unsafe& resource_,
+ total.filter(predicate).resourcesNoMutationWithoutExclusiveOwnership) {
// Need to `toUnreserved` to ignore the roles in contains().
Resources unreserved;
unreserved.add(resource_);
@@ -2012,7 +2046,9 @@ Option<Resources> Resources::find(const Resource& target) const
Resources::operator RepeatedPtrField<Resource>() const
{
RepeatedPtrField<Resource> all;
- foreach (const shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
all.Add()->CopyFrom(resource_->resource);
}
@@ -2103,7 +2139,9 @@ void Resources::add(const Resource_& that)
}
bool found = false;
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (internal::addable(resource_->resource, that.resource)) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -2118,7 +2156,8 @@ void Resources::add(const Resource_& that)
// Cannot be combined with any existing Resource object.
if (!found) {
- resources.push_back(make_shared<Resource_>(that));
+ resourcesNoMutationWithoutExclusiveOwnership.push_back(
+ make_shared<Resource_>(that));
}
}
@@ -2130,7 +2169,9 @@ void Resources::add(Resource_&& that)
}
bool found = false;
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (internal::addable(resource_->resource, that.resource)) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -2147,19 +2188,22 @@ void Resources::add(Resource_&& that)
// Cannot be combined with any existing Resource object.
if (!found) {
- resources.push_back(make_shared<Resource_>(std::move(that)));
+ resourcesNoMutationWithoutExclusiveOwnership.push_back(
+ make_shared<Resource_>(std::move(that)));
}
}
-void Resources::add(const shared_ptr<Resource_>& that)
+void Resources::add(const Resource_Unsafe& that)
{
if (that->isEmpty()) {
return;
}
bool found = false;
- foreach (shared_ptr<Resource_>& resource_, resources) {
+ foreach (
+ Resource_Unsafe& resource_,
+ resourcesNoMutationWithoutExclusiveOwnership) {
if (internal::addable(resource_->resource, that->resource)) {
// Copy-on-write (if more than 1 reference).
if (resource_.use_count() > 1) {
@@ -2174,7 +2218,7 @@ void Resources::add(const shared_ptr<Resource_>& that)
// Cannot be combined with any existing Resource object.
if (!found) {
- resources.push_back(that);
+ resourcesNoMutationWithoutExclusiveOwnership.push_back(that);
}
}
@@ -2217,7 +2261,9 @@ Resources& Resources::operator+=(Resource&& that)
Resources& Resources::operator+=(const Resources& that)
{
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
add(resource_);
}
@@ -2227,7 +2273,9 @@ Resources& Resources::operator+=(const Resources& that)
Resources& Resources::operator+=(Resources&& that)
{
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
add(std::move(resource_));
}
@@ -2257,8 +2305,10 @@ void Resources::subtract(const Resource_& that)
return;
}
- for (size_t i = 0; i < resources.size(); i++) {
- shared_ptr<Resource_>& resource_ = resources[i];
+ for (size_t i = 0; i < resourcesNoMutationWithoutExclusiveOwnership.size();
+ i++) {
+ Resource_Unsafe& resource_ =
+ resourcesNoMutationWithoutExclusiveOwnership[i];
if (internal::subtractable(resource_->resource, that)) {
// Copy-on-write (if more than 1 reference).
@@ -2286,8 +2336,9 @@ void Resources::subtract(const Resource_& that)
// As `resources` is not ordered, and erasing an element
// from the middle is expensive, we swap with the last element
// and then shrink the vector by one.
- resources[i] = resources.back();
- resources.pop_back();
+ resourcesNoMutationWithoutExclusiveOwnership[i] =
+ resourcesNoMutationWithoutExclusiveOwnership.back();
+ resourcesNoMutationWithoutExclusiveOwnership.pop_back();
}
break;
@@ -2316,7 +2367,9 @@ Resources& Resources::operator-=(const Resource& that)
Resources& Resources::operator-=(const Resources& that)
{
- foreach (const shared_ptr<Resource_>& resource_, that.resources) {
+ foreach (
+ const Resource_Unsafe& resource_,
+ that.resourcesNoMutationWithoutExclusiveOwnership) {
subtract(*resource_);
}