Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
6b8b489
add deferred_creation util into stats
stevenzzzz Jun 9, 2023
8f3119a
add missed files
stevenzzzz Jun 9, 2023
17cfb10
update with fixed
stevenzzzz Jun 9, 2023
f8ba76a
move the API changes into to this PR as well
stevenzzzz Jun 9, 2023
68d3c6b
josh comments
stevenzzzz Jun 9, 2023
19d75d8
update comment in deferred_creation.h
stevenzzzz Jun 9, 2023
2bbace4
save proto comment update
stevenzzzz Jun 9, 2023
7d69cbf
update DeferredCreation comment
stevenzzzz Jun 9, 2023
8318935
add comment for why the scope is taken by reference
stevenzzzz Jun 9, 2023
91e47e3
update the extra cost estimation on deferred_creation stats
stevenzzzz Jun 9, 2023
cfc9859
sad no local typo check tool
stevenzzzz Jun 9, 2023
f6bd430
more nits fixing
stevenzzzz Jun 12, 2023
97c2b86
fix typo
stevenzzzz Jun 13, 2023
8e9dfe5
Update source/common/stats/deferred_creation.h
stevenzzzz Jun 13, 2023
bca128a
Update source/common/stats/deferred_creation.h
stevenzzzz Jun 13, 2023
d1ee36c
fix Greg comments
stevenzzzz Jun 13, 2023
448e474
Merge branch 'main' into deferred-1
stevenzzzz Jun 13, 2023
1fa16e7
fix format
stevenzzzz Jun 13, 2023
6e9a861
Create a DeferredStatOptions submessage in bootstrap for possible fut…
stevenzzzz Jun 13, 2023
c1d2aa4
fix the placement issue in bootstrap proto
stevenzzzz Jun 14, 2023
4e72f1e
fixes for format error and nits
stevenzzzz Jun 14, 2023
35e7cb1
make the method public
stevenzzzz Jun 14, 2023
0a11cb5
hmm, fix grammar errors around explicit
stevenzzzz Jun 14, 2023
8a80f36
remove trailing whitespace
stevenzzzz Jun 14, 2023
448074d
Adi comment fix
stevenzzzz Jun 16, 2023
28db9d3
add not-implemented-hide tag
stevenzzzz Jun 20, 2023
8134314
fix doc format: broken ref
stevenzzzz Jun 20, 2023
c22db02
gix doc format
stevenzzzz Jun 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions envoy/stats/stats.h
Original file line number Diff line number Diff line change
Expand Up @@ -218,5 +218,44 @@ using SizeFn = std::function<void(std::size_t)>;
*/
template <typename Stat> using StatFn = std::function<void(Stat&)>;

/**
* Interface for stats lazy initialization.
* To save memory and CPU consumption from unused stats, Envoy can enable the bootstrap config

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest slightly more detail:

To save memory and CPU consumption on blocks of stats that are never referenced throughout the process lifetime, they can be encapsulated in a DeferredCreationCompatibleInterface. Then the Envoy bootstrap configuration can be set to defer the instantiation of those block. Note that when the blocks of stats are created, they carry an extra ~160 byte overhead (depending on worker thread count) due to internal bookkeeping data structures. The overhead when deferred stats are disabled is just 8 bytes.

WDYT? You should confirm the 160 bytes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.

did you see this? #23921 (comment)

"The last column shows the overhead is around 5MB for 100K clusters."

* :ref:`enable_deferred_creation_stats
* <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.enable_deferred_creation_stats>`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put the API update in this PR

@stevenzzzz stevenzzzz Jun 9, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootstrap.proto is in this pR

* A 'StatsStructType' is only created when any of its field is referenced.
* See more context: https://github.com/envoyproxy/envoy/issues/23575
*/
template <typename StatsStructType> class DeferredCreationCompatibleInterface {
public:
// Helper function to get-or-create and return the StatsStructType object.
virtual StatsStructType& instantiate() PURE;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in the comment, I think getOrCreate() may be a better name if that's what this does.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


virtual ~DeferredCreationCompatibleInterface() = default;
};

// Template that lazily initializes a StatsStruct.
// The bootstrap config :ref:`enable_deferred_creation_stats
// <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.enable_deferred_creation_stats>` decides if
// stats lazy initialzation is enabled or not.
template <typename StatsStructType> class DeferredCreation;
template <typename StatsStructType> class DirectStats;

// A helper class for a lazy compatible stats struct type.
template <typename StatsStructType> class DeferredCreationCompatibleStats {
public:
DeferredCreationCompatibleStats(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: explicit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

std::unique_ptr<DeferredCreationCompatibleInterface<StatsStructType>> d)
: data_(std::move(d)) {}
// Allows move construct and assign.
DeferredCreationCompatibleStats& operator=(DeferredCreationCompatibleStats&&) = default;
DeferredCreationCompatibleStats(DeferredCreationCompatibleStats&&) = default;

inline StatsStructType* operator->() { return &data_->instantiate(); };
inline StatsStructType& operator*() { return data_->instantiate(); };

private:
std::unique_ptr<DeferredCreationCompatibleInterface<StatsStructType>> data_;
};
} // namespace Stats
} // namespace Envoy
5 changes: 3 additions & 2 deletions envoy/stats/stats_macros.h
Original file line number Diff line number Diff line change
Expand Up @@ -158,16 +158,17 @@ static inline std::string statPrefixJoin(absl::string_view prefix, absl::string_
*/
#define MAKE_STATS_STRUCT(StatsStruct, StatNamesStruct, ALL_STATS) \
struct StatsStruct { \
using StatNameType = StatNamesStruct; \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand why you need to create this alias. But can you add a comment explaining that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, added a line comment.

static const absl::string_view typeName() { return #StatsStruct; } \
StatsStruct(const StatNamesStruct& stat_names, Envoy::Stats::Scope& scope, \
Envoy::Stats::StatName prefix = Envoy::Stats::StatName()) \
: stat_names_(stat_names) \
ALL_STATS(MAKE_STATS_STRUCT_COUNTER_HELPER_, MAKE_STATS_STRUCT_GAUGE_HELPER_, \
MAKE_STATS_STRUCT_HISTOGRAM_HELPER_, \
MAKE_STATS_STRUCT_TEXT_READOUT_HELPER_, \
MAKE_STATS_STRUCT_STATNAME_HELPER_) {} \
const StatNamesStruct& stat_names_; \
const StatNameType& stat_names_; \
ALL_STATS(GENERATE_COUNTER_STRUCT, GENERATE_GAUGE_STRUCT, GENERATE_HISTOGRAM_STRUCT, \
GENERATE_TEXT_READOUT_STRUCT, GENERATE_STATNAME_STRUCT) \
}

} // namespace Envoy
12 changes: 12 additions & 0 deletions source/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "deferred_creation",
hdrs = ["deferred_creation.h"],
deps = [
"//envoy/stats:stats_interface",
"//source/common/common:cleanup_lib",
"//source/common/common:thread_lib",
"//source/common/stats:symbol_table_lib",
"//source/common/stats:utility_lib",
],
)

envoy_cc_library(
name = "histogram_lib",
srcs = ["histogram_impl.cc"],
Expand Down
112 changes: 112 additions & 0 deletions source/common/stats/deferred_creation.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
#pragma once

#include "envoy/common/pure.h"
#include "envoy/stats/scope.h"
#include "envoy/stats/stats.h"

#include "source/common/common/cleanup.h"
#include "source/common/common/thread.h"
#include "source/common/stats/symbol_table.h"
#include "source/common/stats/utility.h"

namespace Envoy {
namespace Stats {

/**
* Lazy-initialization wrapper for StatsStructType, intended for deferred instantiation of a block
* of stats that might not be needed in a given Envoy process.
*
* This class is thread-safe -- instantiations can occur on multiple concurrent threads.
* This is used when
* :ref:`enable_deferred_creation_stats
* <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.enable_deferred_creation_stats>` is enabled.
*/
template <typename StatsStructType>
class DeferredCreation : public DeferredCreationCompatibleInterface<StatsStructType> {
public:
// Capture the stat names object and the scope with a ctor, that can be used to instantiate a
// StatsStructType object later.
// Caller should make sure scope and stat_names outlive this object.
DeferredCreation(const typename StatsStructType::StatNameType& stat_names,
Stats::ScopeSharedPtr scope)
: initialized_([&scope]() -> Gauge& {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to capture scope by value here?

That this works makes me suspect we don't have a test that removes the scope first and then instantiates the stats.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor accepts the scope by value, so this should be fine, right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually come to think of it, line 41 will cause the scope originally passed in to the constructor to be deleted. Shouldn't we keep the scope alive for the lifetime of DeferredCreation as a member within it and just reference that in ctor_ instead of moving it into it?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the constructor will be out of scope before the lambda gets run, so no help there :(

I think maybe this is all moot because StatsStructType does not hold a reference-count for the scope, but holds it by Scope&. This is because I only (relatively) recently changed scopes to be represented as shared_ptr but didn't change all the Scope& everywhere in Envoy to a ScopeSharedPtr.

So what will happen in the test scenario I just thought of:

ScopeSharedPtr scope = createScope(...);
DeferredCreation deferred_stats(stat_names, scope);
scope.reset();
deferred_stats.instantiate();

I think the code as is will reference freed memory, but with my suggestion it will work, but not be useful :)

It will successfully instantiate the stats in the scope, and then the Cleanup thing will remove the scope. So you'll be left with struct full of invalid stats. So if you then do:

deferred_stats->my_counter_.inc():

that will then crash.

This is beyond the scope of this PR to resolve (sic) but I'd still vote for capturing scope by value on line 32.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scope is used first in initialize the gauge, and then moved into the ctor_, could you help me to understand the life cycle issue there?

the scope is always supposed to outlive the stats, otherwise the stats will be deleted IIUC.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I articulated the lifecycle issue above.

In practice it doesn't matter but my recommendation stands -- just change &scope to scope in your capture.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not following where this "reference freed memory" is from.
the line 32 capture is used and thrown away after init of the initialized_ Gauge.

Scope is held in ctor_ until it's called.
the case raised by @jmarantz should have no problem to call instantiate(), because the scope was captured.

but further references to its member I am not sure, since the last copy of "scope" is deleted.

I have the understanding that scope always outlive the individual StatStruct, if that not true, then the previous suggestion of do not capture scope in DeferredCreation need to be revisited.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lambda in line 32 is not stored anywhere, so capturing scope by value there does not change anything. It's a temporary that is invoked and immediately destroyed.

The lambda that is stored is the one on line 38, and the scope there is (effectively) captured by value (moved from the passed in by value object for the constructor of DeferredCreation.
My point though is that too is destroyed when instantiate is called as ctor_ is set to nullptr when invoked.

However storing the scope in DeferredCreation would also require storing it in DirectStats or else we could see unexpected behaviour in the future based on the value of enableDeferredCreationStats

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the code as is, the call to deferred_stats.instantiate() will work just fine, but deferred_stats->my_counter_.inc() will crash (but this is the same behaviour for direct_stats).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added comment as asked.

Stats::StatNamePool pool(scope->symbolTable());
return Stats::Utility::gaugeFromElements(
*scope, {pool.add(StatsStructType::typeName()), pool.add("initialized")},
Stats::Gauge::ImportMode::HiddenAccumulate);
}()),
ctor_([&, stats_scope = std::move(scope)]() -> StatsStructType* {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/&/this/ ? does that work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

initialized_.inc();
// Reset ctor_ to save some RAM.
Cleanup reset_ctor([&] { ctor_ = nullptr; });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/&/this/ ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return new StatsStructType(stat_names, *stats_scope);
}) {
if (initialized_.value() > 0) {
instantiate();
}
}
~DeferredCreation() {
if (ctor_ == nullptr) {
initialized_.dec();
}
}

private:
inline StatsStructType& instantiate() override { return *internal_stats_.get(ctor_); }

// In order to preserve stat value continuity across a config reload, we need to automatically
// re-instantiate lazy stats when they are constructed, if there is already a live instantiation
// to the same stats. Consider the following alternate scenarios:

// Scenario 1: a cluster is instantiated but receives no requests, so its traffic-related stats
// are never instantiated. When this cluster gets reloaded on a config update, a new lazy-init
// block is created, but the stats should again not be instantiated.

// Scenario 2: a cluster is instantiated and receives traffic, so its traffic-related stats are
// instantiated. We must ensure that a new instance for the same cluster gets its lazy-stats
// instantiated before the previous cluster of the same name is destructed.

// To do that we keep an "initialized" gauge in the cluster's scope, which will be associated by
// name to the previous generation's cluster's lazy-init block. We use the value in this shared
// gauge to determine whether to instantiate the lazy block on construction.
// TODO(#26106): See #14610. The initialized_ gauge could be disabled in a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh, right, thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot disable hidden stats per @DiazAlan so you can remove the TODO starting line 72 also.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

// corner case where a user disables stats with suffix "initialized". In which case, the
// initialized_ will be a NullGauge, which breaks the above scenario 2.
// TODO(#26106): Consider hiding this Gauge from being exported, through using the
// stats flags mask.
Gauge& initialized_;
// TODO(#26957): Clean up this ctor_ by moving its ownership to AtomicPtr, and drop
// the setter lambda when the nested object is created.
std::function<StatsStructType*()> ctor_;
Thread::AtomicPtr<StatsStructType, Thread::AtomicPtrAllocMode::DeleteOnDestruct> internal_stats_;
};

// Non-DeferredCreation wrapper over StatsStructType. This is used when
// :ref:`enable_deferred_creation_stats
// <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.enable_deferred_creation_stats>` is not
// enabled.
template <typename StatsStructType>
class DirectStats : public DeferredCreationCompatibleInterface<StatsStructType> {
public:
DirectStats(const typename StatsStructType::StatNameType& stat_names, Stats::Scope& scope)
: stats_(stat_names, scope) {}

private:
inline StatsStructType& instantiate() override { return stats_; }
StatsStructType stats_;
};

template <typename StatsStructType>
DeferredCreationCompatibleStats<StatsStructType>
createDeferredCompatibleStats(Stats::ScopeSharedPtr scope,
const typename StatsStructType::StatNameType& stat_names,
bool deferred_creation) {
if (deferred_creation) {
return {std::make_unique<DeferredCreation<StatsStructType>>(stat_names, scope)};
} else {
return {std::make_unique<DirectStats<StatsStructType>>(stat_names, *scope)};
}
}

} // namespace Stats
} // namespace Envoy
36 changes: 36 additions & 0 deletions test/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,42 @@ envoy_cc_benchmark_binary(
],
)

envoy_cc_test(
name = "deferred_creation_stats_test",
srcs = ["deferred_creation_stats_test.cc"],
deps = [
"//envoy/stats:stats_interface",
"//source/common/memory:stats_lib",
"//source/common/stats:deferred_creation",
"//source/common/stats:thread_local_store_lib",
"//test/test_common:utility_lib",
],
)

envoy_cc_benchmark_binary(
name = "deferred_creation_stats_benchmark",
srcs = ["deferred_creation_stats_speed_test.cc"],
external_deps = [
"benchmark",
],
deps = [
":real_thread_test_base",
"//source/common/common:random_generator_lib",
"//source/common/common:utility_lib",
"//source/common/runtime:runtime_lib",
"//source/common/stats:deferred_creation",
"//source/common/stats:isolated_store_lib",
"//source/common/stats:symbol_table_lib",
"//source/exe:process_wide_lib",
],
)

envoy_benchmark_test(
name = "deferred_creation_stats_benchmark_test",
size = "large",
benchmark_binary = "deferred_creation_stats_benchmark",
)

envoy_benchmark_test(
name = "symbol_table_benchmark_test",
benchmark_binary = "symbol_table_benchmark",
Expand Down
Loading