ppadevski
commented
2 years ago I will unfortunately will not be able to submit this patch myself, at least not from this account, and I'd have to go through some bureaucracy if I am to submit any code in this repository, so for the time being please do not assign this issue to me and ask if someone else is interested in making these changes.
In the meantime I took another stab and noticed other things that can be initialised lazily, in particular I noticed that we had a ton of std::_Sp_counted_ptr_inplace<bool, std::allocator<bool> allocations and my clusters have neither priority updates nor someone has registered for member updates. The allocations in question actually come from CallbackManager: const std::shared_ptr<bool> still_alive_{std::make_shared<bool>(true)}; and in particular from PrioritySetImpl members host_sets_priority_updatecbs, member_update_cbhelper and priority_update_cbhelper.
If I apply some lazy initialisation which I'll attach at the end of the comment I end up with:
Unsigned chunks have 872063 instances taking 0x3fbbe28(66,829,864) bytes.
Signature 1cf5b80 (std::_Sp_counted_ptr_inplace<bool, std::allocator<bool>, (__gnu_cxx::_Lock_policy)2>) has 90081 instances taking 0x2c1040(2,887,744) bytes.
Signature 1d06d50 (Envoy::Upstream::HostSetImpl) has 85034 instances taking 0x1611a90(23,141,008) bytes.
Signature 1cfb040 (Envoy::Upstream::ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry) has 82533 instances taking 0xa13680(10,565,248) bytes.
Signature 1cea770 (Envoy::Extensions::Clusters::XXX::LoadBalancer) has 82500 instances taking 0x284880(2,640,000) bytes.The remaining std::_Sp_counted_ptr_inplace<bool, std::allocator<bool> instances are probably callbacks for cluster updates which we can probably live with for the time being. The HostSetImpl(s) are still fat and I haven't tried lazy initialising anything in them yet. ClusterEntry(s) have been significantly reduced from 39,615,840 to 10,565,248 bytes and unsigned chunks have dropped with both patches a lot. The XXX::LoadBalancer allocations bother me a bit as they are all instances of a custom stateless LoadBalancer for which I end up having a lot of allocations for but only need one.
The second patch:
diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc
index 4f2beb1..a9f096e 100644
--- a/source/common/upstream/upstream_impl.cc
+++ b/source/common/upstream/upstream_impl.cc
@@ -548,11 +548,13 @@ PrioritySetImpl::getOrCreateHostSet(uint32_t priority,
if (host_sets_.size() < priority + 1) {
for (size_t i = host_sets_.size(); i <= priority; ++i) {
HostSetImplPtr host_set = createHostSet(i, overprovisioning_factor);
- host_sets_priority_update_cbs_.push_back(
- host_set->addPriorityUpdateCb([this](uint32_t priority, const HostVector& hosts_added,
- const HostVector& hosts_removed) {
- runReferenceUpdateCallbacks(priority, hosts_added, hosts_removed);
- }));
+ if (lazy_) {
+ lazy_->host_sets_priority_update_cbs_.push_back(
+ host_set->addPriorityUpdateCb([this](uint32_t priority, const HostVector& hosts_added,
+ const HostVector& hosts_removed) {
+ runReferenceUpdateCallbacks(priority, hosts_added, hosts_removed);
+ }));
+ }
host_sets_.push_back(std::move(host_set));
}
}
diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h
index dfe0090..4a9c853 100644
--- a/source/common/upstream/upstream_impl.h
+++ b/source/common/upstream/upstream_impl.h
@@ -463,11 +463,25 @@ public:
// From PrioritySet
ABSL_MUST_USE_RESULT Common::CallbackHandlePtr
addMemberUpdateCb(MemberUpdateCb callback) const override {
- return member_update_cb_helper_.add(callback);
+ if (!member_update_cb_helper_) {
+ member_update_cb_helper_ =
+ std::make_unique<Common::CallbackManager<const HostVector&, const HostVector&>>();
+ }
+ return member_update_cb_helper_->add(callback);
}
ABSL_MUST_USE_RESULT Common::CallbackHandlePtr
addPriorityUpdateCb(PriorityUpdateCb callback) const override {
- return priority_update_cb_helper_.add(callback);
+ if (!lazy_) {
+ lazy_ = std::make_unique<Lazy>();
+ for (auto& host_set: host_sets_) {
+ lazy_->host_sets_priority_update_cbs_.push_back(
+ static_cast<HostSetImpl*>(host_set.get())->addPriorityUpdateCb([this](
+ uint32_t priority, const HostVector& hosts_added, const HostVector& hosts_removed) {
+ runReferenceUpdateCallbacks(priority, hosts_added, hosts_removed);
+ }));
+ }
+ }
+ return lazy_->priority_update_cb_helper_.add(callback);
}
const std::vector<std::unique_ptr<HostSet>>& hostSetsPerPriority() const override {
return host_sets_;
@@ -493,11 +507,15 @@ protected:
protected:
virtual void runUpdateCallbacks(const HostVector& hosts_added, const HostVector& hosts_removed) {
- member_update_cb_helper_.runCallbacks(hosts_added, hosts_removed);
+ if (member_update_cb_helper_) {
+ member_update_cb_helper_->runCallbacks(hosts_added, hosts_removed);
+ }
}
virtual void runReferenceUpdateCallbacks(uint32_t priority, const HostVector& hosts_added,
- const HostVector& hosts_removed) {
- priority_update_cb_helper_.runCallbacks(priority, hosts_added, hosts_removed);
+ const HostVector& hosts_removed) const {
+ if (lazy_) {
+ lazy_->priority_update_cb_helper_.runCallbacks(priority, hosts_added, hosts_removed);
+ }
}
// This vector will generally have at least one member, for priority level 0.
// It will expand as host sets are added but currently does not shrink to
@@ -505,13 +523,18 @@ protected:
std::vector<std::unique_ptr<HostSet>> host_sets_;
private:
- // This is a matching vector to store the callback handles for host_sets_. It is kept separately
- // because host_sets_ is directly returned so we avoid translation.
- std::vector<Common::CallbackHandlePtr> host_sets_priority_update_cbs_;
- // TODO(mattklein123): Remove mutable.
- mutable Common::CallbackManager<const HostVector&, const HostVector&> member_update_cb_helper_;
- mutable Common::CallbackManager<uint32_t, const HostVector&, const HostVector&>
- priority_update_cb_helper_;
+ mutable std::unique_ptr<Common::CallbackManager<const HostVector&, const HostVector&>> member_update_cb_helper_;
+
+ struct Lazy {
+ // This is a matching vector to store the callback handles for host_sets_. It is kept separately
+ // because host_sets_ is directly returned so we avoid translation.
+ std::vector<Common::CallbackHandlePtr> host_sets_priority_update_cbs_;
+ // TODO(mattklein123): Remove mutable.
+ Common::CallbackManager<uint32_t, const HostVector&, const HostVector&> priority_update_cb_helper_;
+ };
+
+ mutable std::unique_ptr<Lazy> lazy_;
+
bool batch_update_ : 1;
// Helper class to maintain state as we perform multiple host updates. Keeps track of all hosts
mattklein123
jmarantz
wbpcode
rojkov
tpetkov-VMW
alyssawilk
Title: Reduce used cluster memory by initialising http_asyncclient lazily
Description: I've been trying to put my envoy on a diet and noticed that most (if not all) of my clusters do not seemingly use httpAsyncClient() but I am paying a price for it in terms of used memory. I have 2500 TLS clusters and by default I end up with about ~249.7m used memory in idle state. If I were to make http_asyncclient lazy given the below patch I end up with ~209.7m used memory which is a huge difference. Below is the patch I used for measuring though I haven't tested it yet if it works for clusters that actually need the httpAsyncClient().
I'll look around and see what else can be initialised lazily.