locality zone name is looked up often when running with --use-fake-symbol-table 0

[rgs1]

When disabling the fake symbol table, I see the following:

$ while :; do curl -s 0:9901/stats/recentlookups ; echo ; sleep 1 ; done
....
   36778 zone_name_X
   36913 zone_name_Y
   36979 zone_name_Z
...

Note: those numbers aren't going up that fast, it's a few thousand every couple of seconds. Also, it doesn't show up in CPU/mem perf profiling. But it might still be good to clean it up.

What we probably want is to avoid creating a new Stats::StatNameManagedStorage every time a HostDescription instance is constructed:

https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/upstream_impl.cc#L257

We could have a std::unordered_map<std::string, StatName> in PriorityStateManager, and the just pass a reference to the StatName every time a HostImpl is created here:

https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/upstream_impl.cc#L1210

Again, I don't think this is a blocker for changing the default value on the --use-fake-symbol-table flag.

cc: @jmarantz

[jmarantz]

on it

[jmarantz]

This turns out to be a little messier than anticipated. @rgs1 suggested above we could store a map in the PriorityStateManager, but is one of those always available when we make HostDescriptionImpls? I started to make that change but found the fanin of contexts growing. I'll take a look more later with fresh start.

Also, I think as @rgs1 observed this is not really mission-critical because these don't get created in the request flow, IIUC.

[jmarantz]

@mattklein123 could use your advice on which of the many places (dozens?) HostImpl is instantiated this is most likely to be. Ideally we could capture a StatNameSet with a structure we populate when we learn about new hosts (e.g. on an EDS update), and then never reference the hostname again by string.

But because there are so many instances where Hostimpl is instantiated, and I'm not sure they all are triggered by xDS, I might lull myself into thinking I've fixed this problem when I might only have captured the names one level in the call-stack higher but no less frequently.

Thanks for your help!

[mattklein123]

I would assume it's during the EDS update process? cc @snowp

[snowp]

I would imagine the main flow is through EDS updates, though new HostImpl would also be created during other discovery flows (e.g. STRICT_DNS), in which case it would happen during CDS update

[jmarantz]

I'm considering making the zone-name a dynamic segment so we just avoid this issue and the huge refactor to keep zone->StatName maps available for all callers.

Switching to a dynamic segment is (for the impl) one-word change:

diff --git a/source/common/upstream/upstream_impl.h b/source/common/upstream/upstream_impl.h
index 372716d078..57fca9a2b6 100644
--- a/source/common/upstream/upstream_impl.h
+++ b/source/common/upstream/upstream_impl.h
@@ -149,7 +149,7 @@ protected:
   mutable absl::Mutex metadata_mutex_;
   MetadataConstSharedPtr metadata_ ABSL_GUARDED_BY(metadata_mutex_);
   const envoy::config::core::v3::Locality locality_;
-  Stats::StatNameManagedStorage locality_zone_stat_name_;
+  Stats::StatNameDynamcStorage locality_zone_stat_name_;
   mutable HostStats stats_;
   Outlier::DetectorHostMonitorPtr outlier_detector_;
   HealthCheckHostMonitorPtr health_checker_;

A future refactor could make it feasible to track a symbolic StatName for each zone through the call-sites but I think this makes sense for now. SG?

[rgs1]

I like it -- I don't think introducing the zone --> StatName maps is worth the complexity at this point. If switching to a dynamic segment makes the lookups go away (at the expense of some allocations maybe -- not that it matters, not a hot path anyway), I think we should do it.