feat(major): [sc-23696] replace jemalloc dependency with custom malloc interposer#333
Open
supersonicbyte wants to merge 36 commits into
Open
feat(major): [sc-23696] replace jemalloc dependency with custom malloc interposer#333supersonicbyte wants to merge 36 commits into
supersonicbyte wants to merge 36 commits into
Conversation
hassila
requested changes
Sep 1, 2025
hassila
left a comment
hassila
left a comment
Contributor
There was a problem hiding this comment.
Some initial feedback, will ping you offline about the initial bootstrap problem...
Comment on lines
+34
to
+37
| /// Number of total mallocs | ||
| case mallocCountTotal | ||
| /// Number of totatl free calls | ||
| case freeCountTotal |
Contributor
There was a problem hiding this comment.
I think we should keep the small and large malloc count metrics, I would suggest that we put the threshold for what is large/small to match the page size (and document it as such, so e.g. on macOS/ARM it'd be 16KB, while on Linux x86 it'd be 4K). Removing these metrics would be unnecessarily source breaking.
Comment on lines
127
to
129
|
|
||
| let parameterization = (0...5).map { 1 << $0 } // 1, 2, 4, ... | ||
| let parameterization = (0...5).map { 1 << $0 } // 1, 2, 4, ... | ||
|
|
Contributor
There was a problem hiding this comment.
Please merge in .swift-format from main and apply swift-format, then we can remove a lot of these diffs with extra spaces.
| @@ -0,0 +1,24 @@ | |||
| // swift-tools-version: 6.1 | |||
Contributor
There was a problem hiding this comment.
Should move to "swift-tools-version: 5.10" everywhere, as we still support 5.10.
|
|
||
| if mallocStatsRequested { | ||
| startMallocStats = MallocStatsProducer.makeMallocStats() | ||
| MallocInterposerSwift.hook() |
Contributor
There was a problem hiding this comment.
You don't want to hook in this closure, it can be called twice as mentioned in the above comment - it is also unnecessary overhead to hook/unhook for each benchmark iteration, better to do the hooking outside of the loop - see how it's done for ARCStatsProducer which also hooks/unhooks, but does it outside the loop.
|
|
||
| if mallocStatsRequested { | ||
| stopMallocStats = MallocStatsProducer.makeMallocStats() | ||
| MallocInterposerSwift.unhook() |
Contributor
There was a problem hiding this comment.
Same here, you want to unhook outside of the loop...
Contributor
|
Also I see a difference in which metrics are output between main and this branch, e.g. : Main: this PR: Got some additional malloc related metrics there, would be good to understand root cause. |
Contributor
|
(that was from running |
Contributor
|
Also one issue when testing with package-benchmarks-samples repo with lost capture of allocation: Main gives: PR: |
Contributor
|
(Memory (allocated resident) (K) was missing from the PR output too?) |
Contributor
|
This test also misses capture: PR: |
Contributor
|
And this test should show a leak, but does not on the PR: PR: |
Contributor
Author
Thanks for the feedback! That's strange will look into it.. |
Contributor
|
This one failed to find the Malloc/free delta leak on Linux: |
Contributor
|
For Linux: cp .build/x86_64-unknown-linux-gnu/debug/libMallocInterposerC-tool.so .build/x86_64-unknown-linux-gnu/debug/libMallocInterposerC.so
cp .build/x86_64-unknown-linux-gnu/debug/libMallocInterposerSwift-tool.so .build/x86_64-unknown-linux-gnu/debug/libMallocInterposerSwift.so(need to rebuild in between too, so build, first copy, build, second copy, ...) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #333 +/- ##
==========================================
- Coverage 68.85% 68.76% -0.08%
==========================================
Files 33 33
Lines 3152 3147 -5
==========================================
- Hits 2170 2164 -6
- Misses 982 983 +1
... and 2 files with indirect coverage changes
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
This was referenced Sep 3, 2025
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Contributor
Author
|
I will take the time to cherry pick these into a future release. The cherry pick could be a bit tricky because we depend on a lot of changes which are not cherry picked. |
Comment on lines
+239
to
+245
| void *replacement_reallocf(void *ptr, size_t size) { | ||
| void *new_ptr = replacement_realloc(ptr, size); | ||
| if (!new_ptr) { | ||
| replacement_free(new_ptr); | ||
| } | ||
| return new_ptr; | ||
| } |
There was a problem hiding this comment.
Critical bug in replacement_reallocf: When realloc fails, it should free the original pointer (ptr), not new_ptr (which is NULL). The function reallocf is designed to free the original memory on failure to prevent memory leaks.
void *replacement_reallocf(void *ptr, size_t size) {
void *new_ptr = replacement_realloc(ptr, size);
if (!new_ptr) {
replacement_free(ptr); // Should free ptr, not new_ptr
}
return new_ptr;
}
Suggested change
| void *replacement_reallocf(void *ptr, size_t size) { | |
| void *new_ptr = replacement_realloc(ptr, size); | |
| if (!new_ptr) { | |
| replacement_free(new_ptr); | |
| } | |
| return new_ptr; | |
| } | |
| void *replacement_reallocf(void *ptr, size_t size) { | |
| void *new_ptr = replacement_realloc(ptr, size); | |
| if (!new_ptr) { | |
| replacement_free(ptr); | |
| } | |
| return new_ptr; | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Comment on lines
+20
to
+22
| linkerSettings: [ | ||
| .linkedLibrary("dl") | ||
| ]) |
There was a problem hiding this comment.
The dl library is Linux-specific and doesn't exist on macOS. This will cause build failures on macOS. The linker setting should be conditional:
linkerSettings: [
.linkedLibrary("dl", .when(platforms: [.linux]))
]
Suggested change
| linkerSettings: [ | |
| .linkedLibrary("dl") | |
| ]) | |
| linkerSettings: [ | |
| .linkedLibrary("dl", .when(platforms: [.linux])) | |
| ]) |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Contributor
Author
|
Great news is that all the dependencies we need are merged in for the next release of swift package manager (6.3.0) which should hopefully come out in March! We can test it by:
|
3 tasks
Contributor
Author
|
Currently blocked by this: swiftlang/swift#87696 (comment) |
Comment on lines
+272
to
+276
| void *replacement_calloc(size_t count, size_t size) { | ||
| void *ptr = replacement_malloc(count * size); | ||
| memset(ptr, 0, count * size); | ||
| return ptr; | ||
| } |
There was a problem hiding this comment.
Critical bug: NULL pointer dereference. If replacement_malloc fails and returns NULL, memset will be called on a NULL pointer causing a segmentation fault.
void *replacement_calloc(size_t count, size_t size) {
void *ptr = replacement_malloc(count * size);
if (ptr) { // Fix: check for NULL before using
memset(ptr, 0, count * size);
}
return ptr;
}
Suggested change
| void *replacement_calloc(size_t count, size_t size) { | |
| void *ptr = replacement_malloc(count * size); | |
| memset(ptr, 0, count * size); | |
| return ptr; | |
| } | |
| void *replacement_calloc(size_t count, size_t size) { | |
| void *ptr = replacement_malloc(count * size); | |
| if (ptr) { | |
| memset(ptr, 0, count * size); | |
| } | |
| return ptr; | |
| } | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
Contributor
|
Pull request had an unknown failure |
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
2 participants
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.
Description
This PR replaces
jemallocwith a custom malloc interposer. The interposer logic is mostly inspired by the great work done by theswift-nioteam.On macOS/Darwin we ulitise the interposing feature of dydl while on Linux we use
LD_PRELOADin order to interpose.The interposition logic is written in C and located in the
MallocInterposerCpackage.For actual interaction with the interposition is done through a wrapper package
MallocInterposerSwift, which exposes a simple interface tohookandunhookthe interposition logic and also to actually count the statistics we need.The interposition requirers the the interposer library to be linked dynamically, since that's crucial to the nature of how the interposition works (both on Linux and Darwin). The SwiftPM Command plugin
BenchmarkCommandPluginhas a dependency onBenchmarkwhich has a dependency on the interposer libs. This causes a problem for SwiftPM and intitial tiggers ofswift package benchmarkcommand will fail.The issue is described in more detail here.
The current workaround is to trigger the
swift package benchmarkcommand once and let it fail. Then to copy thelibMallocInterposerC-tool.dylibandlibMallocInterposerSwift-tool.dyliband name them without the-toolsuffix.e.g.
The same applies for Linux but the build location will be different and the dynamic library extension is
.so.Since jemalloc is removed the
mallocSmallandmallocLargemetrics are removed. New metricsmallocByteCountandfreeTotalCountare introducted.How Has This Been Tested?
When running the test the executable is being linked statically to the test target. I didn't discover a way yet to dynamically link the interposer in order for it to work inside tests.
Minimal checklist:
DocCcode-level documentation for any public interfaces exported by the package