Fix cross-thread free statistics - #1411
Open
B Navneshwar (Navneshwar) wants to merge 1 commit into
Open
B Navneshwar (Navneshwar) wants to merge 1 commit into
B Navneshwar (Navneshwar) wants to merge 1 commit into
Conversation
Author
|
@microsoft-github-policy-service agree |
|
Hi, thanks for working on this. I don't think this is the correct way to fix this. Doing atomic updates on Also, there is now a disconnect where allocations are credited lazily, but deallocations from other threads happen immediately. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 increasingmalloc_normal.currentvalue 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:
theap.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:
After both worker threads exited, the reported statistics returned to approximately the actual live memory:
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 suppliedtheap.For cross-thread frees,
theapcan represent the currently executing thread even though the page belongs to another thread.Previously, the free path used:
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:The free-stat updates now use
free_theap:This gives the following behavior:
theapaccounting.NULLfor thetheap, causing the statistics update to go to the shared heap statistics.This keeps the change narrowly scoped to the problematic cross-thread free accounting path.
Regression test
A regression test named
stats-cross-thread-freewas added totest/test-api.c.The test:
malloc_normal.current.theap.mi_stats_get()is called.malloc_normal.currentdecreased by exactly the usable size of the freed allocation.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
Build completed successfully.
Regression test
Result:
Full test suite
Result:
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:
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.ctest/test-api.c