diff --git a/.gitlab/reliability/chaos_check.sh b/.gitlab/reliability/chaos_check.sh index 844a8522b..22b99e130 100755 --- a/.gitlab/reliability/chaos_check.sh +++ b/.gitlab/reliability/chaos_check.sh @@ -123,6 +123,11 @@ case $ALLOCATOR in tcmalloc) echo "Running with tcmalloc" export LD_PRELOAD=$(find /usr/lib/ -name 'libtcmalloc_minimal.so.4') + # thread-churn/dump-storm antagonists cycle many short-lived threads; + # tcmalloc's defaults are slow to return their per-thread caches to the + # OS, which was inflating container RSS past the OOM limit on aarch64. + export TCMALLOC_RELEASE_RATE=10 + export TCMALLOC_AGGRESSIVE_DECOMMIT=1 ;; jemalloc) echo "Running with jemalloc" diff --git a/ddprof-lib/src/main/cpp/counters.h b/ddprof-lib/src/main/cpp/counters.h index 655013664..4ef2d5ae1 100644 --- a/ddprof-lib/src/main/cpp/counters.h +++ b/ddprof-lib/src/main/cpp/counters.h @@ -42,6 +42,7 @@ X(DICTIONARY_ENDPOINTS_ARENA_WASTE_BYTES, "dictionary_endpoints_arena_waste_bytes") \ X(DICTIONARY_CONTEXT_ARENA_WASTE_BYTES, "dictionary_context_arena_waste_bytes") \ X(DICTIONARY_DRAIN_TIMEOUTS, "dictionary_drain_timeouts") \ + X(CLASS_MAP_AT_CAPACITY, "class_map_at_capacity_drops") \ X(CONTEXT_STORAGE_BYTES, "context_storage_bytes") \ X(CONTEXT_STORAGE_PAGES, "context_storage_pages") \ X(CONTEXT_BOUNDS_MISS_INITS, "context_bounds_miss_inits") \ diff --git a/ddprof-lib/src/main/cpp/flightRecorder.cpp b/ddprof-lib/src/main/cpp/flightRecorder.cpp index 1bd4ec1bc..c219bf40d 100644 --- a/ddprof-lib/src/main/cpp/flightRecorder.cpp +++ b/ddprof-lib/src/main/cpp/flightRecorder.cpp @@ -68,7 +68,7 @@ SharedLineNumberTable::~SharedLineNumberTable() { void Lookup::fillNativeMethodInfo(MethodInfo *mi, const char *name, const char *lib_name) { - mi->_class = _classes->lookupDuringDump("", 0); + mi->_class = _classes->lookupDuringDump("", 0, Profiler::maxClassMapSize()); // TODO return the library name once we figured out how to cooperate with the // backend // if (lib_name == NULL) { @@ -117,7 +117,8 @@ void Lookup::fillNativeMethodInfo(MethodInfo *mi, const char *name, void Lookup::fillRemoteFrameInfo(MethodInfo *mi, const RemoteFrameInfo *rfi) { // Store build-id in the class name field - mi->_class = _classes->lookupDuringDump(rfi->build_id, strlen(rfi->build_id)); + mi->_class = _classes->lookupDuringDump(rfi->build_id, strlen(rfi->build_id), + Profiler::maxClassMapSize()); // Store PC offset in hex format in the signature field char offset_hex[32]; @@ -269,7 +270,8 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method, "Ljdk/internal/reflect/GeneratedConstructorAccessor")) { class_name_id = _classes->lookupDuringDump( "jdk/internal/reflect/GeneratedConstructorAccessor", - strlen("jdk/internal/reflect/GeneratedConstructorAccessor")); + strlen("jdk/internal/reflect/GeneratedConstructorAccessor"), + Profiler::maxClassMapSize()); method_name_id = _symbols.lookup("Object " "jdk.internal.reflect.GeneratedConstructorAccessor." @@ -277,18 +279,20 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method, method_sig_id = _symbols.lookup(method_sig); } else if (has_prefix(class_name, "Lsun/reflect/GeneratedConstructorAccessor")) { - class_name_id = - _classes->lookupDuringDump("sun/reflect/GeneratedConstructorAccessor", - strlen("sun/reflect/GeneratedConstructorAccessor")); + class_name_id = _classes->lookupDuringDump( + "sun/reflect/GeneratedConstructorAccessor", + strlen("sun/reflect/GeneratedConstructorAccessor"), + Profiler::maxClassMapSize()); method_name_id = _symbols.lookup( "Object " "sun.reflect.GeneratedConstructorAccessor.newInstance(Object[])"); method_sig_id = _symbols.lookup(method_sig); } else if (has_prefix(class_name, "Ljdk/internal/reflect/GeneratedMethodAccessor")) { - class_name_id = - _classes->lookupDuringDump("jdk/internal/reflect/GeneratedMethodAccessor", - strlen("jdk/internal/reflect/GeneratedMethodAccessor")); + class_name_id = _classes->lookupDuringDump( + "jdk/internal/reflect/GeneratedMethodAccessor", + strlen("jdk/internal/reflect/GeneratedMethodAccessor"), + Profiler::maxClassMapSize()); method_name_id = _symbols.lookup("Object " "jdk.internal.reflect.GeneratedMethodAccessor." @@ -296,8 +300,10 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method, method_sig_id = _symbols.lookup(method_sig); } else if (has_prefix(class_name, "Lsun/reflect/GeneratedMethodAccessor")) { - class_name_id = _classes->lookupDuringDump("sun/reflect/GeneratedMethodAccessor", - strlen("sun/reflect/GeneratedMethodAccessor")); + class_name_id = _classes->lookupDuringDump( + "sun/reflect/GeneratedMethodAccessor", + strlen("sun/reflect/GeneratedMethodAccessor"), + Profiler::maxClassMapSize()); method_name_id = _symbols.lookup( "Object sun.reflect.GeneratedMethodAccessor.invoke(Object, " "Object[])"); @@ -308,30 +314,38 @@ void Lookup::fillJavaMethodInfo(MethodInfo *mi, jmethodID method, // we want to normalise to java/lang/invoke/LambdaForm$MH, // java/lang/invoke/LambdaForm$DMH, java/lang/invoke/LambdaForm$BMH, if (has_prefix(class_name + lambdaFormPrefixLength, "MH")) { - class_name_id = _classes->lookupDuringDump("java/lang/invoke/LambdaForm$MH", - strlen("java/lang/invoke/LambdaForm$MH")); + class_name_id = _classes->lookupDuringDump( + "java/lang/invoke/LambdaForm$MH", + strlen("java/lang/invoke/LambdaForm$MH"), + Profiler::maxClassMapSize()); } else if (has_prefix(class_name + lambdaFormPrefixLength, "BMH")) { - class_name_id = _classes->lookupDuringDump("java/lang/invoke/LambdaForm$BMH", - strlen("java/lang/invoke/LambdaForm$BMH")); + class_name_id = _classes->lookupDuringDump( + "java/lang/invoke/LambdaForm$BMH", + strlen("java/lang/invoke/LambdaForm$BMH"), + Profiler::maxClassMapSize()); } else if (has_prefix(class_name + lambdaFormPrefixLength, "DMH")) { - class_name_id = _classes->lookupDuringDump("java/lang/invoke/LambdaForm$DMH", - strlen("java/lang/invoke/LambdaForm$DMH")); + class_name_id = _classes->lookupDuringDump( + "java/lang/invoke/LambdaForm$DMH", + strlen("java/lang/invoke/LambdaForm$DMH"), + Profiler::maxClassMapSize()); } else { // don't recognise the suffix, so don't normalise class_name_id = _classes->lookupDuringDump( - normalized_class_name, normalized_class_name_len); + normalized_class_name, normalized_class_name_len, + Profiler::maxClassMapSize()); } method_name_id = _symbols.lookup(method_name); method_sig_id = _symbols.lookup(method_sig); } else { - class_name_id = _classes->lookupDuringDump(normalized_class_name, - normalized_class_name_len); + class_name_id = _classes->lookupDuringDump( + normalized_class_name, normalized_class_name_len, + Profiler::maxClassMapSize()); method_name_id = _symbols.lookup(method_name); method_sig_id = _symbols.lookup(method_sig); } } else { Counters::increment(JMETHODID_SKIPPED); - class_name_id = _classes->lookupDuringDump("", 0); + class_name_id = _classes->lookupDuringDump("", 0, Profiler::maxClassMapSize()); method_name_id = _symbols.lookup("jvmtiError"); method_sig_id = _symbols.lookup("()L;"); } @@ -426,7 +440,7 @@ bool Lookup::resolveVTableReceiver(VMSymbol *sym, char *buf, size_t bufsize, // the stack frame's class_id absent from this chunk's class pool. // (Plain lookup() remains correct for non-dump callers — e.g. Profiler:: // lookupClass on JVM threads — where the next rotate() will propagate.) - u32 class_id = _classes->lookupDuringDump(buf, len); + u32 class_id = _classes->lookupDuringDump(buf, len, Profiler::maxClassMapSize()); // Apply synthetic-accessor/LambdaForm normalisation so that the many // distinct names HotSpot generates for these families (..Accessor1234, // LambdaForm$MH/0x...) collapse to one bucket each in the JFR class pool. @@ -436,32 +450,32 @@ bool Lookup::resolveVTableReceiver(VMSymbol *sym, char *buf, size_t bufsize, if (has_prefix_n(buf, len, "jdk/internal/reflect/GeneratedConstructorAccessor")) { static const char kName[] = "jdk/internal/reflect/GeneratedConstructorAccessor"; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } else if (has_prefix_n(buf, len, "sun/reflect/GeneratedConstructorAccessor")) { static const char kName[] = "sun/reflect/GeneratedConstructorAccessor"; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } else if (has_prefix_n(buf, len, "jdk/internal/reflect/GeneratedMethodAccessor")) { static const char kName[] = "jdk/internal/reflect/GeneratedMethodAccessor"; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } else if (has_prefix_n(buf, len, "sun/reflect/GeneratedMethodAccessor")) { static const char kName[] = "sun/reflect/GeneratedMethodAccessor"; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } else if (has_prefix_n(buf, len, "java/lang/invoke/LambdaForm$")) { size_t prefix_len = strlen("java/lang/invoke/LambdaForm$"); const char *suffix = buf + prefix_len; size_t suffix_len = len - prefix_len; if (suffix_len >= 2 && suffix[0] == 'M' && suffix[1] == 'H') { static const char kName[] = "java/lang/invoke/LambdaForm$MH"; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } else if (suffix_len >= 3 && suffix[0] == 'B' && suffix[1] == 'M' && suffix[2] == 'H') { static const char kName[] = "java/lang/invoke/LambdaForm$BMH"; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } else if (suffix_len >= 3 && suffix[0] == 'D' && suffix[1] == 'M' && suffix[2] == 'H') { static const char kName[] = "java/lang/invoke/LambdaForm$DMH"; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } } *out_class_id = class_id; @@ -485,7 +499,7 @@ u32 Lookup::resolveVTableReceiverCached(void *sym) { // marker instead of an empty class name (which is indistinguishable // from a parser/encoder error downstream). static const char kName[] = ""; - class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1); + class_id = _classes->lookupDuringDump(kName, sizeof(kName) - 1, Profiler::maxClassMapSize()); } _vtable_receiver_cache[sym] = class_id; return class_id; diff --git a/ddprof-lib/src/main/cpp/livenessTracker.cpp b/ddprof-lib/src/main/cpp/livenessTracker.cpp index ac471463c..fdf9130f1 100644 --- a/ddprof-lib/src/main/cpp/livenessTracker.cpp +++ b/ddprof-lib/src/main/cpp/livenessTracker.cpp @@ -117,12 +117,19 @@ void LivenessTracker::flush_table(std::set *tracked_thread_ids) { env->DeleteLocalRef(clz); jniExceptionCheck(env); const char *name = env->GetStringUTFChars(name_str, nullptr); - event._id = name != nullptr - ? Profiler::instance()->lookupClass(name, strlen(name)) - : 0; + int class_id = name != nullptr + ? Profiler::instance()->lookupClass(name, strlen(name)) + : 0; env->ReleaseStringUTFChars(name_str, name); - Profiler::instance()->recordDeferredSample(_table[i].tid, _table[i].call_trace_id, BCI_LIVENESS, &event); + // lookupClass() returns -1 when the class map is at capacity; do not + // assign it to the u32 event id (it would wrap to 0xFFFFFFFF and + // corrupt liveness attribution in the JFR output) — drop the sample + // instead, matching ObjectSampler's convention for the same failure. + if (class_id >= 0) { + event._id = class_id; + Profiler::instance()->recordDeferredSample(_table[i].tid, _table[i].call_trace_id, BCI_LIVENESS, &event); + } } env->DeleteLocalRef(ref); diff --git a/ddprof-lib/src/main/cpp/profiler.cpp b/ddprof-lib/src/main/cpp/profiler.cpp index 561009e22..05dc7ea62 100644 --- a/ddprof-lib/src/main/cpp/profiler.cpp +++ b/ddprof-lib/src/main/cpp/profiler.cpp @@ -1832,10 +1832,16 @@ void Profiler::shutdown(Arguments &args) { } int Profiler::lookupClass(const char *key, size_t length) { - // StringDictionary::lookup() is internally thread-safe via _accepting + - // RefCountGuard; no external lock required (unlike the old Dictionary). - u32 id = _class_map.lookup(key, length); - return id != 0 ? static_cast(id) : -1; + // StringDictionary::bounded_lookup() is internally thread-safe via + // _accepting + RefCountGuard; no external lock required. Bounded to + // MAX_CLASS_MAP_SIZE so unbounded distinct-class churn cannot grow this + // dictionary for the lifetime of the process. + u32 id = _class_map.bounded_lookup(key, length, MAX_CLASS_MAP_SIZE); + if (id == 0) { + Counters::increment(CLASS_MAP_AT_CAPACITY); + return -1; + } + return static_cast(id); } int Profiler::status(char* status, int max_len) { diff --git a/ddprof-lib/src/main/cpp/profiler.h b/ddprof-lib/src/main/cpp/profiler.h index 2f13d2a3a..0e4369fdb 100644 --- a/ddprof-lib/src/main/cpp/profiler.h +++ b/ddprof-lib/src/main/cpp/profiler.h @@ -81,6 +81,15 @@ class alignas(alignof(SpinLock)) Profiler { NotifyClassUnloadedFunc _notify_class_unloaded_func; // -- + // Caps the number of distinct class names _class_map will ever hold. + // rotate()/clearStandby() carry the full accumulated set forward on every + // JFR chunk (only clearAll() on profiler restart truly resets it), so + // without a cap a workload with unbounded distinct-class churn (e.g. heavy + // dynamic class generation) grows this dictionary for the lifetime of the + // process. 256K distinct classes is far beyond any real application's + // class count. + static const int MAX_CLASS_MAP_SIZE = 1 << 18; + ThreadInfo _thread_info; StringDictionary _class_map{1}; StringDictionary _string_label_map{2}; @@ -262,6 +271,10 @@ class alignas(alignof(SpinLock)) Profiler { Engine *wallEngine() { return _wall_engine; } StringDictionary *classMap() { return &_class_map; } + // Same cap lookupClass() enforces for sample-time inserts; dump-time + // insertions (Lookup::_classes->lookupDuringDump in flightRecorder.cpp) + // must use this too so _class_map cannot grow past it via that path. + static int maxClassMapSize() { return MAX_CLASS_MAP_SIZE; } SharedLockGuard classMapSharedGuard() { return SharedLockGuard(&_class_map_lock); } StringDictionary *stringLabelMap() { return &_string_label_map; } StringDictionary *contextValueMap() { return &_context_value_map; } diff --git a/ddprof-lib/src/main/cpp/stringDictionary.h b/ddprof-lib/src/main/cpp/stringDictionary.h index 05bb62d3d..a87b7ec0f 100644 --- a/ddprof-lib/src/main/cpp/stringDictionary.h +++ b/ddprof-lib/src/main/cpp/stringDictionary.h @@ -12,6 +12,7 @@ #include "tripleBuffer.h" #include "arch.h" #include +#include #include #include #include @@ -550,7 +551,11 @@ class StringDictionary { // Resolve a key during the dump phase. Safe to call from the dump thread // after rotate(); must NOT be called from signal handlers or concurrently // with another lookupDuringDump call. - u32 lookupDuringDump(const char* key, size_t len) { + // size_limit bounds new insertions the same way bounded_lookup() does: + // once active->size() reaches size_limit, previously-known keys still + // resolve (dump/active hits above), but a genuinely new key returns 0 + // instead of growing the dictionary further. + u32 lookupDuringDump(const char* key, size_t len, int size_limit = INT_MAX) { StringDictionaryBuffer* dump = _rot.dumpBuffer(); u32 id = dump->lookup(key, len); @@ -571,6 +576,7 @@ class StringDictionary { StringDictionaryBuffer* active = _rot.active(); RefCountGuard guard(active); if (!guard.isActive()) return 0; + if (active->size() >= size_limit) return 0; u32 new_id = nextId(); new_id = active->insert_with_id(key, len, new_id); if (new_id != 0) dump->insert_with_id(key, len, new_id); diff --git a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp index 814e9b4ca..e0117af6a 100644 --- a/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp +++ b/ddprof-lib/src/test/cpp/livenessTracker_ut.cpp @@ -210,6 +210,54 @@ TEST_F(LivenessTrackerTest, MultipleResizeOperationsMaintainCorrectCapacity) { EXPECT_EQ(mock.getCapacity(), 128); } +/** + * Mock structure to test the flush_table id-assignment guard: Profiler::lookupClass() + * returns an int (-1 on class-map-at-capacity), but Event::_id is a u32. Assigning -1 + * directly would wrap to 0xFFFFFFFF and corrupt liveness attribution, so flush_table + * must drop the sample instead. Mirrors ObjectSampler's convention for the same + * lookupClass() failure mode. + */ +struct FlushTableIdGuardMock { + bool recorded = false; + uint32_t recorded_id = 0; + + // Correct behavior (after the fix): only assign/record when class_id >= 0. + void applyGuarded(int class_id) { + if (class_id >= 0) { + recorded = true; + recorded_id = static_cast(class_id); + } + } + + // Pre-fix behavior: unconditionally assigns class_id to the u32 event id. + void applyUnguarded(int class_id) { + recorded = true; + recorded_id = static_cast(class_id); + } +}; + +TEST_F(LivenessTrackerTest, NegativeClassIdSampleIsDropped) { + FlushTableIdGuardMock mock; + mock.applyGuarded(-1); + EXPECT_FALSE(mock.recorded); +} + +TEST_F(LivenessTrackerTest, NonNegativeClassIdSampleIsRecorded) { + FlushTableIdGuardMock mock; + mock.applyGuarded(42); + EXPECT_TRUE(mock.recorded); + EXPECT_EQ(42u, mock.recorded_id); +} + +TEST_F(LivenessTrackerTest, UnguardedNegativeClassIdWrapsToMaxU32) { + // Documents the bug the guard prevents: without it, -1 wraps to 0xFFFFFFFF + // when narrowed to the u32 event id. + FlushTableIdGuardMock mock; + mock.applyUnguarded(-1); + EXPECT_TRUE(mock.recorded); + EXPECT_EQ(0xFFFFFFFFu, mock.recorded_id); +} + /** * Test that verifies capacity never exceeds max_cap during resize operations. */ diff --git a/ddprof-lib/src/test/cpp/stringDictionary_ut.cpp b/ddprof-lib/src/test/cpp/stringDictionary_ut.cpp index aaad96243..554418f39 100644 --- a/ddprof-lib/src/test/cpp/stringDictionary_ut.cpp +++ b/ddprof-lib/src/test/cpp/stringDictionary_ut.cpp @@ -257,6 +257,24 @@ TEST_F(StringDictionaryTest, ClearAllResetsEverything) { EXPECT_EQ(1u, new_id); } +TEST_F(StringDictionaryTest, BoundedLookupWithSizeLimitRejectsNewKeyAtCapacity) { + // Fill active up to size_limit distinct keys via the capped overload. + const int size_limit = 5; + for (int i = 0; i < size_limit; i++) { + std::string k = "cap_" + std::to_string(i); + u32 id = dict.bounded_lookup(k.c_str(), k.size(), size_limit); + EXPECT_GT(id, 0u) << "insert " << i << " should have succeeded below the limit"; + } + + // A brand-new key must be rejected once active->size() >= size_limit. + EXPECT_EQ(0u, dict.bounded_lookup("overflow", 8, size_limit)); + + // An already-present key must still resolve to its existing id even at capacity + // (cache hit is checked before the capacity check). + u32 id0 = dict.bounded_lookup("cap_0", 5, size_limit); + EXPECT_GT(id0, 0u); +} + TEST_F(StringDictionaryTest, LookupDuringDumpInsertsNewKeyIntoActiveAndStandby) { dict.rotate(); // empty active becomes dump, fresh active // Key is not in dump and not in active — lookupDuringDump must insert into both. @@ -271,3 +289,24 @@ TEST_F(StringDictionaryTest, LookupDuringDumpInsertsNewKeyIntoActiveAndStandby) // Must be in active (bounded_lookup is a probe of active) EXPECT_EQ(id, dict.bounded_lookup("brand/New", 9)); } + +TEST_F(StringDictionaryTest, LookupDuringDumpWithSizeLimitRejectsNewKeyAtCapacity) { + dict.rotate(); // empty active becomes dump, fresh active + + // Fill active up to size_limit distinct keys via the capped overload. + const int size_limit = 5; + for (int i = 0; i < size_limit; i++) { + std::string k = "dump_cap_" + std::to_string(i); + u32 id = dict.lookupDuringDump(k.c_str(), k.size(), size_limit); + EXPECT_GT(id, 0u) << "insert " << i << " should have succeeded below the limit"; + } + + // A brand-new key must be rejected once active->size() >= size_limit — + // neither active nor the dump snapshot should gain the entry. + EXPECT_EQ(0u, dict.lookupDuringDump("dump_overflow", 13, size_limit)); + + // An already-present key must still resolve to its existing id even at + // capacity (dump/active hits are checked before the capacity check). + u32 id0 = dict.lookupDuringDump("dump_cap_0", 10, size_limit); + EXPECT_GT(id0, 0u); +}