xiangfu0 commented on code in PR #20147: URL: https://github.com/apache/pinot/pull/20147#discussion_r4466240062
########## pinot-core/src/main/java/org/apache/pinot/core/accounting/GcStateDescriber.java: ########## @@ -0,0 +1,139 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.core.accounting; + +import com.google.common.annotations.VisibleForTesting; +import com.sun.management.GcInfo; +import java.lang.management.GarbageCollectorMXBean; +import java.lang.management.ManagementFactory; +import java.lang.management.MemoryPoolMXBean; +import java.lang.management.MemoryType; +import java.lang.management.MemoryUsage; +import java.util.List; +import java.util.Locale; +import java.util.function.LongSupplier; +import java.util.stream.Collectors; +import javax.annotation.Nullable; + + +/// Describes the JVM garbage collection state for the heap-based query kill logs. +/// +/// The used heap that the accountant compares against its thresholds includes garbage that the next collection will +/// free. The description adds what is needed to tell a heap full of live data from a heap full of garbage: +/// - The old generation usage after the last collection, which is the best cheap estimate of the live set. For G1 the +/// JVM updates it after every collection, young ones included. +/// - For each collector, the number of collections and how long ago the last one ended. +/// +/// The class keeps no state. It reads the MXBeans only when [#describe()] is called, which happens on level +/// transitions and kills, not on every sample. +public class GcStateDescriber { + // Heap pool names that identify the old generation: "G1 Old Gen", "PS Old Gen", "Tenured Gen", + // "ZGC Old Generation" + private static final List<String> OLD_POOL_MARKERS = List.of("old", "tenured"); + // Heap pool names that identify the young generation, used when no pool matches the old markers + private static final List<String> YOUNG_POOL_MARKERS = List.of("eden", "survivor", "young", "nursery"); + // Declared after the markers, which the constructor reads + private static final GcStateDescriber INSTANCE = new GcStateDescriber(ManagementFactory.getGarbageCollectorMXBeans(), + ManagementFactory.getMemoryPoolMXBeans(), ManagementFactory.getRuntimeMXBean()::getUptime); + + private final List<GarbageCollectorMXBean> _collectors; + private final List<MemoryPoolMXBean> _oldGenPools; + private final LongSupplier _uptimeMs; + + @VisibleForTesting + GcStateDescriber(List<GarbageCollectorMXBean> collectors, List<MemoryPoolMXBean> pools, LongSupplier uptimeMs) { + _collectors = collectors; + _oldGenPools = findOldGenPools(pools); + _uptimeMs = uptimeMs; + } + + public static GcStateDescriber getInstance() { + return INSTANCE; + } + + /// Returns a one-line description of the GC state, for example: + /// `oldGenUsedAfterLastGcBytes: 49392123904, collectors: [G1 Young Generation (count: 1644482, last ended 120 ms + /// ago), G1 Concurrent GC (count: 3042472, last ended 1300 ms ago), G1 Old Generation (count: 154, last ended + /// 3400 ms ago)]` + public String describe() { + try { + return "oldGenUsedAfterLastGcBytes: " + getOldGenUsedAfterLastGcBytes() + ", collectors: " + + _collectors.stream().map(this::describe).collect(Collectors.joining(", ", "[", "]")); + } catch (RuntimeException e) { + // The description is only diagnostic. Never let it break the kill path. + return "unavailable: " + e; + } + } + + /// Returns the old generation usage right after the last collection, or -1 when the JVM does not report it. + @VisibleForTesting + long getOldGenUsedAfterLastGcBytes() { + long usedBytes = 0; + boolean reported = false; + for (MemoryPoolMXBean pool : _oldGenPools) { + MemoryUsage collectionUsage = pool.getCollectionUsage(); Review Comment: On G1, `G1 Old Gen#getCollectionUsage()` is **not** updated after pure young GCs. HotSpot only updates it after mixed, full, and concurrent-cycle collections. I ran this PR's `GcStateDescriber` on G1 with `-Xmx1g`, JDK 25.0.5 (same on 21.0.12), retaining small objects so they get promoted: ``` oldGenUsedNow=285031912 -> oldGenUsedAfterLastGcBytes: 0, collectors: [G1 Young Generation (count: 13, last ended 35 ms ago), G1 Concurrent GC (count: 0), ...] ``` So in the scenario from the PR description (young GCs only, between concurrent cycles), the log reports a tiny or stale live set while the old gen is full of live data. The young collector's `getLastGcInfo().getMemoryUsageAfterGc().get("G1 Old Gen")` does track every young GC: it gave 35/61/83/…/305 MB in the same probe. One fix: use the old-pool "after GC" usage from whichever collector managing that pool has the latest `GcInfo.getEndTime()`. Lines 39-40 of the javadoc would also need updating. ########## pinot-core/src/test/java/org/apache/pinot/core/accounting/GcStateDescriberTest.java: ########## @@ -0,0 +1,122 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.pinot.core.accounting; + +import com.sun.management.GcInfo; +import java.lang.management.GarbageCollectorMXBean; +import java.lang.management.MemoryPoolMXBean; +import java.lang.management.MemoryType; +import java.lang.management.MemoryUsage; +import java.util.List; +import java.util.stream.Collectors; +import javax.annotation.Nullable; +import org.testng.annotations.Test; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.testng.Assert.assertEquals; + + +/// Feeds [GcStateDescriber] with fake MXBeans. The collector and pool names are the ones each JDK collector reports. +public class GcStateDescriberTest { + private static final long UPTIME_MS = 1_000_000; + + @Test + public void testDescribeG1() { + List<MemoryPoolMXBean> pools = List.of(pool("G1 Eden Space", MemoryType.HEAP, 0L), + pool("G1 Survivor Space", MemoryType.HEAP, 10L), pool("G1 Old Gen", MemoryType.HEAP, 400L), Review Comment: This test assumes the old pool's collection usage (400) is current after 10 young GCs. Real G1 reports 0 here until a concurrent, mixed, or full collection runs, so the test locks in the wrong assumption. It's worth asserting against `GcInfo#getMemoryUsageAfterGc` from the young collector instead, if the implementation changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
