feat(minor): Disable jemalloc using package traits#347
feat(minor): Disable jemalloc using package traits#347supersonicbyte merged 10 commits intoordo-one:mainfrom
Conversation
|
This PR title should have been marked with feat: I can't seem to edit the PR title however. @hassila |
|
Thanks @JaapWijnen - but we will soon remove the jemalloc dependency completely through #333 - although that will require a 6.3 toolchain - so maybe worth landing this for 6.0-6.2 support regardless - @JaapWijnen what is you earliest supported toolchain at this point? @supersonicbyte over to you |
I agree this is probably worth landing for 6.0 - 6.2. Will do an extensive PR review soon. |
|
Oh that's great! |
|
Yeah, when 6.3 lands we will bump this package to 6.1-6.2 for the old jemalloc and 6.3 for the new malloc interposer (that requires bug fixes from SPM that only exist in 6.3...) |
|
Ah I see. Awesome. Yeah we'll be moving to 6.3 as soon as it lands so that's great to hear :) Thanks! |
|
@JaapWijnen everything looks good! just a quick comment. In order to fix the build for linux we should add #if canImport(Darwin)
@preconcurrency import Darwin
#elseif canImport(Glibc)
@preconcurrency import Glibc
#elseif canImport(Musl)
@preconcurrency import Musl
#else
#error("Unsupported Platform")
#endifRegarding the crash when running It's a bug that happens when xctest runs with jemalloc. |
@supersonicbyte Done! |
|
@JaapWijnen thanks! I removed the One more thing, @hassila actually noticed that the trait In order to preserve backward compatibility could you flip the logic so:
Additionally, we can also rename the trait to Sorry for the additional comment, I should have seen this in the first review pass. |
|
Good point! All done 👍 |
targets Benchmark and BenchmarkTool have to be set to language mode v5 due to concurrency errors.
# Conflicts: # Package.resolved
3b9d577 to
2566488
Compare
|
Anything else you need me to do? :) @supersonicbyte |
I fixed the workflow files so our CI passes and also changed the README to reflect the changes, hope you don't mind! Thank you for your contribution, merging it right now! |
|
Oh this is great. Thanks for taking that on! Now that this is merged. When do you think the breaking changes will be merged/tagged? (edit: Oh right that's only supported once 6.3 is release) |
## Description With PR #347 we added trait support and we changed the `#canImport(jemalloc)` to `#if Jemalloc`. Since the older toolchains will execute the `Package@5.9` and in there we do not have a way to enable the trait the code guarded with the #if will never compile, the #else block will execute and we will get the stub implementation of the malloc counter which return 0. Quick fix is to revert back to use `#canImport`. Related issue: #350 ## How Has This Been Tested? Please describe the tests that you ran to verify your changes. ## Minimal checklist: - [ ] I have performed a self-review of my own code - [ ] I have added `DocC` code-level documentation for any public interfaces exported by the package - [ ] I have added unit and/or integration tests that prove my fix is effective or that my feature works
Description
This is a first attempt at enabling/disable jemalloc using package traits #316
This shows off what a trait based implementation could look like. But I'm not exactly sure what the expected behaviour would be on unsupported platforms. So that probably needs some more discussion.
How Has This Been Tested?
I performed a build and ran the tests with and without the trait enabled.
The build succeeds on Mac but running tests (with the trait enabled) fails with a segmentation error.
I'm not entirely sure where this is coming from but seems to fail once we add
package-jemallocto the package dependencies. Regardless of if it is actually used by package-benchmark this seems to crash runningswift test --traits EnableJemallocThis user experience should be improved ofcourse. Which I'm hoping to discuss in the PR.
Minimal checklist:
DocCcode-level documentation for any public interfaces exported by the package