[#17375] Add metrics around MemTracker::GcTcmalloc #28777

kenju · 2025-10-02T17:08:43Z

Resolves #17375

Changes Made:

Adds three metrics to track MemTracker::GcTcmallocIfNeeded() performance, addressing the "5% total time" impact on scan queries mentioned in #17375.

GC Call Frequency

METRIC_DEFINE_counter(server, mem_tracker_gc_tcmalloc_calls,
                      "MemTracker GC TCMalloc Calls",
                      yb::MetricUnit::kOperations,
                      "Number of times MemTracker::GcTcmallocIfNeeded() has been called");

~~2. Total Memory Released~~
(Edited) this was dropped as mem_tracker_gc_tcmalloc_bytes_per_gc_sum, which actually reports the value with mem_tracker_gc_tcmalloc_bytes_per_gc_sum. See #28777 (comment) for more details.

Original proposal for `mem_tracker_gc_tcmalloc_bytes_released`

METRIC_DEFINE_counter(server, mem_tracker_gc_tcmalloc_bytes_released,
                      "MemTracker GC TCMalloc Bytes Released",
                      yb::MetricUnit::kBytes,
                      "Total number of bytes released by MemTracker::GcTcmallocIfNeeded()");

Distribution of Memory Released Per GC

METRIC_DEFINE_histogram(server, mem_tracker_gc_tcmalloc_bytes_per_gc,
                        "MemTracker GC TCMalloc Bytes Per Call",
                        yb::MetricUnit::kBytes,
                        "Histogram of bytes released per call to MemTracker::GcTcmallocIfNeeded()",
                        60000000LU, 2);

Test Verified:

Verified in the following set up:

Multi-node clusters started
Prometheus/Grafana set up to scrape from /prometheud-metrics endpoint
Created an ad-hod dashboard
Doing a couple of load testing over bin/ysqlsh doing INSERT/SELECT queries against large dataset

Here is a screenshot:

kenju · 2025-10-02T17:09:24Z

@bmatican (@spolitov @timothy-e) 👋🏻 I've raised a quick PR based on the comment here #17375 (comment) Can I get your quick review please?

kenju · 2025-10-02T17:10:20Z

src/yb/util/mem_tracker.cc

+                        "MemTracker GC TCMalloc Bytes Per Call",
+                        yb::MetricUnit::kBytes,
+                        "Histogram of bytes released per call to MemTracker::GcTcmallocIfNeeded()",
+                        60000000 /* 60MB as max value */, 2 /* 2 digits precision */);


☝🏻 just a hypothetical number which is large enough to accommodate most of real world use cases, would love to hear your feedback if this needs to be updated

Let's use 60000000LU

Good catch thanks ✅ 6108969

netlify · 2025-10-02T17:10:27Z

✅ Deploy Preview for infallible-bardeen-164bc9 ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`bb449dd`
🔍 Latest deploy log	https://app.netlify.com/projects/infallible-bardeen-164bc9/deploys/68ef9d45f43f9f00082c9b74
😎 Deploy Preview	https://deploy-preview-28777--infallible-bardeen-164bc9.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

kenju · 2025-10-02T17:11:19Z

src/yb/yql/pggate/pggate.cc

      ybctid_reader_provider_(pg_session_),
      fk_reference_cache_(ybctid_reader_provider_, buffering_settings_, tablespace_map_),
      explicit_row_lock_buffer_(ybctid_reader_provider_, tablespace_map_) {
+  MemTracker::InitializeGcMetrics(metric_entity_);


#17375 (comment)

This is primarily useful for the tserver, but could also be valuable for PG,

If I understand it correctly, we'd like to initialise metrics here as well for PG

kenju · 2025-10-03T18:43:10Z

@rthallamko3 Another PR for improving metrics - can I get a reviewer for this PR please?

yusong-yan · 2025-10-14T19:48:09Z

src/yb/util/mem_tracker.cc

+    int64_t final_overhead = GetTCMallocPageHeapFreeBytes();
+    int64_t bytes_released = initial_overhead - final_overhead;
+    if (gc_tcmalloc_bytes_released_metric_ && bytes_released > 0) {
+      gc_tcmalloc_bytes_released_metric_->IncrementBy(bytes_released);
+    }
+    if (gc_tcmalloc_bytes_per_call_metric_ && bytes_released > 0) {
+      gc_tcmalloc_bytes_per_call_metric_->Increment(bytes_released);
+    }


bytes_released could theoretically be negative if memory increased during GC (unlikely but possible due to concurrent allocations). The > 0 check prevents recording negative values, which is good.
I think we should add a comment to indicate that the metric doesn't perfectly record number of byte is released(as well as the metric description)

(unlikely but possible due to concurrent allocations)

Good point. I can naively come up with following scenarios:

Time T0: initial_overhead = 100MB (measured before GC) Time T1: GC starts, releases 50MB from page heap Time T2: Other threads allocate 80MB Time T3: final_overhead = 130MB (measured after GC) (Results) bytes_released = 100MB - 130MB = -30MB

I believe the current approach still give us some pictures around GC than having no metrics at all.

Added code comments & improve metrics description ✅ 04ca6f7

yusong-yan

Nice work overall! I added a few comments for consideration. Really appreciate your contribution and the effort you put into this!

yusong-yan · 2025-10-14T20:02:59Z

src/yb/util/mem_tracker.cc

+    if (gc_tcmalloc_bytes_per_call_metric_) {
+      gc_tcmalloc_bytes_per_call_metric_->Increment(0);
+    }


Do you think it might make more sense to change the metric semantics to represent “bytes released per GC” instead of “bytes released per call”?
Since we record 0 even when no GC happens, if there are many such calls, it could skew the histogram (especially the average and percentiles) toward 0, making it harder to interpret the real GC behavior.

it might make more sense to change the metric semantics to represent “bytes released per GC” instead of “bytes released per call”?

Stepping back a little bit, I think that's right - we're interested in GC behaviour and not particularly in GcTcmallocIfNeeded() calls anyway. Thanks for the feedback!

Stopped recording 0 to avoid skewing the aggregation result ✅ 9f6c439

…concurrent allocations

kenju · 2025-10-15T15:58:27Z

@yusong-yan Thanks for having a look and dropping all the great feedback! I've made an improvement and replied back in each comment threads. Let me know if there is anything else I need to fix.

…_gc_tcmalloc_bytes_per_gc_sum

kenju · 2025-10-15T18:57:38Z

I discussed with @yusong-yan in Slack, and we decided to drop mem_tracker_gc_tcmalloc_bytes_released as this is the same value reporting with mem_tracker_gc_tcmalloc_bytes_per_gc_sum. ✅ 10bc2e7

Confirmed from my dev environment as well:

$ curl --silent http://localhost:9000/prometheus-metrics | grep mem_tracker_gc_tcmalloc_ | grep -v "^#"

mem_tracker_gc_tcmalloc_bytes_released{metric_id="yb.tabletserver",metric_type="server"} 146882560 1760554158881
mem_tracker_gc_tcmalloc_bytes_per_gc_sum{metric_type="server",metric_id="yb.tabletserver"} 146882560 1760554158881

kenju commented Oct 2, 2025

View reviewed changes

ddorian requested a review from yusong-yan October 9, 2025 17:33

yusong-yan reviewed Oct 14, 2025

View reviewed changes

yusong-yan requested changes Oct 14, 2025

View reviewed changes

add tcmalloc metrics

bb449dd

kenju force-pushed the kenju/17375-mem-tracker-gc-metrics.md branch from e0c9036 to bb449dd Compare October 15, 2025 13:10

kenju added 3 commits October 15, 2025 14:14

use LU (unsigned long) suffix for metrics max value

6108969

add comment on byte_released edge cases on being negative in case of …

04ca6f7

…concurrent allocations

track bytes released per GC, not per method calls

9f6c439

kenju requested a review from yusong-yan October 15, 2025 15:57

drop gc_tcmalloc_bytes_released_metric_ as this is dup of mem_tracker…

10bc2e7

…_gc_tcmalloc_bytes_per_gc_sum

fix lint error for too-long line length

b2e1673

[#17375] Add metrics around MemTracker::GcTcmalloc #28777

Are you sure you want to change the base?

[#17375] Add metrics around MemTracker::GcTcmalloc #28777

Uh oh!

Conversation

kenju commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes Made:

Test Verified:

Uh oh!

kenju commented Oct 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

netlify bot commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for infallible-bardeen-164bc9 ready!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenju commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yusong-yan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenju commented Oct 15, 2025

Uh oh!

kenju commented Oct 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kenju commented Oct 2, 2025 •

edited

Loading

netlify bot commented Oct 2, 2025 •

edited

Loading