Skip to content

Fix cross-thread free statistics - #1411

Open
B Navneshwar (Navneshwar) wants to merge 1 commit into
microsoft:main3from
Navneshwar:main3
Open

B Navneshwar (Navneshwar) wants to merge 1 commit into
microsoft:main3from
Navneshwar:main3

Conversation

@Navneshwar

Copy link
Copy Markdown

Summary

Fix incorrect mi_stats_get() accounting when a thread frees memory that was allocated by another thread.

When the freeing thread has its own theap, cross-thread frees can currently be attributed to that thread's local statistics. Those statistics are only merged into the heap statistics during collection or thread exit. If the freeing thread rarely collects, mi_stats_get() can therefore report a rapidly increasing malloc_normal.current value even though the actual amount of live memory remains small.

This change routes statistics for cross-thread frees directly to the shared heap statistics while preserving the existing per-thread accounting for same-thread frees and leaving allocation accounting unchanged.

Fixes #1410.

Problem

Issue #1410 demonstrates the problem with a producer/consumer workload:

  1. One thread allocates memory.
  2. Another thread frees that memory.
  3. The consumer thread performs a small allocation/free first, which initializes its theap.
  4. The consumer then repeatedly frees memory allocated by the producer.
  5. The consumer remains alive and does not perform a collection.
  6. mi_stats_get() is called periodically from the main thread.

With the existing implementation, the freed bytes can accumulate in the consumer thread's local statistics instead of being reflected immediately in the shared heap statistics.

The reported statistics can therefore drift far away from the actual amount of live memory.

For example, the original reproducer produced values such as:

t=1s  stats =    29124.2 MiB   actually live =   8.2 MiB
t=2s  stats =    56792.1 MiB   actually live =  10.1 MiB
t=3s  stats =    84789.0 MiB   actually live =  10.4 MiB
t=4s  stats =   114232.9 MiB   actually live =  11.0 MiB

After both worker threads exited, the reported statistics returned to approximately the actual live memory:

after both threads exited: 8.0 MiB (live 8.0 MiB, rss 15.7 MiB)

This indicates that the accumulated per-thread statistics were eventually merged, but were not being reflected in the shared statistics while the freeing thread remained alive.

Root cause

mi_theap_page_merge_stats() receives both allocation and free counts for a page and updates the corresponding statistics using the supplied theap.

For cross-thread frees, theap can represent the currently executing thread even though the page belongs to another thread.

Previously, the free path used:

mi_theapx_stat_decrease(heap, theap, malloc_normal, freed);
mi_theapx_stat_decrease(heap, theap, malloc_bins[bin], free_count);

This means the freed bytes could remain in the freeing thread's local statistics until that thread performed a collection or exited.

Fix

The fix introduces a separate free_theap:

mi_theap_t* const free_theap =
  (mi_page_thread_id(page) == _mi_prim_thread_id() ? theap : NULL);

The free-stat updates now use free_theap:

mi_theapx_stat_decrease(heap, free_theap, malloc_normal, freed);
mi_theapx_stat_decrease(heap, free_theap, malloc_bins[bin], free_count);

This gives the following behavior:

  • Same-thread free:
    • continues to use the existing theap accounting.
  • Cross-thread free:
    • uses NULL for the theap, causing the statistics update to go to the shared heap statistics.
  • Allocation accounting:
    • remains unchanged.
  • Other statistics:
    • remain unchanged.

This keeps the change narrowly scoped to the problematic cross-thread free accounting path.

Regression test

A regression test named stats-cross-thread-free was added to test/test-api.c.

The test:

  1. Creates a producer thread.
  2. Allocates a 4096-byte block on the producer thread.
  3. Records the current malloc_normal.current.
  4. Creates a separate consumer thread.
  5. Initializes the consumer thread's theap.
  6. Frees the producer's allocation from the consumer thread.
  7. Keeps the consumer thread alive while mi_stats_get() is called.
  8. Verifies that malloc_normal.current decreased by exactly the usable size of the freed allocation.
  9. Releases and joins the consumer thread.

The test is guarded with #if !defined(_WIN32) because it uses POSIX pthread synchronization primitives.

Validation

The change was validated against the current mimalloc checkout with:

Build

cmake --build build -j2

Build completed successfully.

Regression test

./build/mimalloc-test-api

Result:

succeeded: 54
failed   : 0

Full test suite

ctest --test-dir build --output-on-failure

Result:

100% tests passed, 0 tests failed out of 8

Original issue reproducer

The original issue reproducer was run against the modified library.

Before the fix, the reproducer showed rapidly increasing statistics reaching more than 100 GiB while actual live memory remained around 10 MiB.

After the fix, the reported statistics remained close to the actual live-memory range and no longer exhibited the runaway growth:

t=1s  stats =       -8.7 MiB   actually live =   8.4 MiB
t=2s  stats =      -28.3 MiB   actually live =   9.2 MiB
t=3s  stats =      -12.9 MiB   actually live =  12.0 MiB
t=4s  stats =       -2.2 MiB   actually live =   7.9 MiB
after both threads exited: 8.0 MiB (live 8.0 MiB, rss 15.6 MiB)

The important change is that the previous runaway accumulation is no longer present, and the final statistics converge to the actual live memory after the worker threads exit.

Files changed

  • src/page.c

    • Route cross-thread free statistics to the shared heap statistics.
  • test/test-api.c

    • Add a regression test for cross-thread free statistics.

@Navneshwar

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

@molnsona

Copy link
Copy Markdown

Hi, thanks for working on this. I don't think this is the correct way to fix this.

Doing atomic updates on heap->stats this often seems like it might cause atomic contention.

Also, there is now a disconnect where allocations are credited lazily, but deallocations from other threads happen immediately.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

malloc_normal.current / malloc_huge.current drifts when a thread rarely allocates and frees other threads' memory periodically

2 participants