tracy icon indicating copy to clipboard operation
tracy copied to clipboard

Fix memory leak in TracyVector for non-trivially-destructible types

Open chaowyc opened this issue 3 months ago • 1 comments

Summary

This PR fixes a memory leak in TracyVector where element destructors are not called for non-trivially-destructible types. The bug affects production code using nested containers like Vector<Vector<short_ptr<ZoneEvent>>>.

Root Cause

TracyVector is designed as a lightweight container optimized for POD types. However, it's also used with non-trivially-destructible types throughout Tracy's codebase. The current implementation never calls element destructors, causing memory leaks when elements manage resources (e.g., heap-allocated memory in nested vectors).

Solution

Add compile-time type checking using C++17 if constexpr with std::is_trivially_destructible_v<T>:

~Vector() {
    if (m_ptr) {
        // Smart destruction - call destructors for non-trivial types
        // For trivial types, if constexpr is optimized away by compiler, zero overhead
        if constexpr (!std::is_trivially_destructible_v<T>) {
            for (uint32_t i = 0; i < m_size; i++) {
                m_ptr[i].~T();
            }
        }
        free(m_ptr);
    }
}

This approach:

  • Zero overhead for trivial types - if constexpr is evaluated at compile time
  • Proper cleanup for non-trivial types - destructors are called when needed
  • Backward compatible - doesn't change behavior for existing POD usage
  • Applied to 4 locations: destructor, move assignment, clear(), Realloc()

Impact on Production Code

This bug affects critical data structures in TracyWorker.hpp:

// Line ~500
Vector<Vector<short_ptr<ZoneEvent>>> zoneChildren;

// Line ~520
Vector<Vector<short_ptr<GpuEvent>>> gpuChildren;

// Line ~540
Vector<Vector<GhostZone>> ghostChildren;

// Line ~560
Vector<Vector<short_ptr<ZoneEvent>>> zoneVectorCache;

TracyVector Usage Statistics:

  • 25+ uses in TracyEvent.hpp (mostly POD types)
  • 4 critical nested container instances in TracyWorker.hpp (affected by this bug)

Verification

Tested with AddressSanitizer/LeakSanitizer using Tracy's actual production types:

Before Fix

=================================================================
==12345==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 24 byte(s) in 3 allocation(s) from:
    #0 0x7f8b4c in malloc
    #1 0x401234 in Vector<Vector<short_ptr<ZoneEvent>>>::push_back

Direct leak of 12 byte(s) in 3 allocation(s) from:
    #0 0x7f8b4c in malloc
    #1 0x401567 in Vector<short_ptr<ZoneEvent>>::push_back

SUMMARY: AddressSanitizer: 36 byte(s) leaked in 6 allocation(s).

After Fix

=================================================================
==12346==LeakSanitizer has found no leaks.
SUMMARY: AddressSanitizer: 0 byte(s) leaked in 0 allocation(s).

Test Coverage

The fix includes a comprehensive test suite (test_before_after_fix.cpp) with 6 test cases:

  1. Nested vectors (Vector<Vector<short_ptr<ZoneEvent>>>)
  2. Move assignment for nested vectors
  3. clear() with non-trivial types
  4. POD types (zero overhead verification)
  5. Complex nested structures (Vector<Vector<GhostZone>>)
  6. Memory reallocation scenarios

All tests pass with AddressSanitizer validation.

Backward Compatibility

  • ✅ No breaking changes
  • ✅ Existing POD usage unchanged (compile-time optimization)
  • ✅ No performance regression for trivial types
  • ✅ Fixes undefined behavior for non-trivial types

Files Changed

  • server/TracyVector.hpp: Added <type_traits> include and if constexpr checks in 4 locations

Request for Feedback

I'd appreciate feedback on:

  1. Whether this approach aligns with Tracy's design philosophy
  2. Any additional test cases you'd like to see
  3. Performance implications (though compile-time checks should have zero overhead)

Thank you for maintaining this excellent profiling tool!

chaowyc avatar Nov 12 '25 21:11 chaowyc

While this is a valid issue (there are indeed leaks caused by tracy::Vector), the implementation of this PR (obviously made by an AI) is incomplete. For example in Vector::insert we use memmove which is not ok if the object is not trivially movable.

In general, it seems Vector should either be restricted to trivially destructible & trivially movable types or changed to fully support them.

As far as I'm aware, the primary interest of using tracy::Vector over std::vector (also used extensively in the codebase) is its "magic" support used when deserializing and the fact it is small (11bytes + 1byte alignment) and saves memory when there are many.

I would personally just add static asserts to detect types that may cause leaks/issues, replace the currently problematic uses by std::vector (they do not benefit much from using Vector, it's fine if we have std::vector<Vector<short_ptr*>> to keep the low memory overhead), but I'll let @wolfpld weigh in on such things.

Lectem avatar Nov 28 '25 17:11 Lectem