search

purpleprotocol / mimalloc_rust

A Rust wrapper over Microsoft's MiMalloc memory allocator
MIT License
481 stars 42 forks source link

Added mimalloc dev and dev-slice support #75

Closed BlackDex closed 2 years ago

BlackDex commented 2 years ago

Resolves #62 Closes #66

It would be really nice if this PR could be merged so that we are able to use the updated mimalloc code from the dev branch, or even the v2 version dev-slice. The master branch is the stable v1 branch and thus does not contain the latest patches.

Both the dev and dev-slice branches have updated code so that it will work correctly on MUSL ARM platforms (see https://github.com/microsoft/mimalloc/issues/495).

With the added feature of executing a git submodule update users of this crate can also be sure that they are able to use the latest commits from upstream mimalloc and not depend on updates from this crate/repo it self if they really want to.

I know that the mimalloc lib isn't fully ABI stable yet, but the current commits for both dev and dev-slice work just fine using this crate, and i have added a warning within the comments.

Thanks in advance!

BlackDex commented 2 years ago

I fixed the cargo fmt error.

BlackDex commented 2 years ago

@octavonce, is this something on which you are open to merge? Or most likely never going to happen?

Because if you are, I will update the code and try to make it all build/pass. Else I'm more inclined to close this PR.

BlackDex commented 2 years ago

@octavonce I think I fixed the CI workflows. I added it in this PR, but if you do not wish to add the dev and dev-slice support, feel free to cherry-pick the CI Fixed commit. Or if you really prefer, i could make a separated PR if you want.

BlackDex commented 2 years ago

@octavonce any feedback on this pr??

happypsyduck commented 2 years ago

I'm also interested in seeing this merged as dev slice (ie: version 2) has memory features that would benefit heavy usage.

BlackDex commented 2 years ago

@octavonce shall i create a separate PR for the workflow fixes? And what do you think about having an option that developers could, if they want to select a custom tag or hash which they want to use instead of adding multiple features?

So, i was thinking about having a feature like custom_checkout, if enabled, it will search for a ENV variable and checks out that version, and uses that to build the static library.

Just add some good comments that people are on there own using that, and that it's not supported/stable. That way you can give a bit flexibility, and still have a nice and stable version without to much changes?

thomcc commented 2 years ago

The submodule_update stuff (and any other part of this that requires git operations) doesn't actually work when published to crates.io. The mimalloc_rust git repo no longer exists after packaging, and more importantly, the mimalloc git repository in the submodule no longer exists after packaging either -- just the files contained in it. So the git operations you try to do will fail.

You can verify this by checking ~/.cargo/registry/src/github.com-1ecc6299db9ec823/libmimalloc-sys-0.1.25/ and seeing that it is not a git repository. Similarly, cargo package -p libmimalloc-sys will produce a target/package/libmimalloc-sys-0.1.25 directory (and a target/package/libmimalloc-sys-0.1.25.crate -- this is what's actually uploaded to the registry, but it's essentially just a tarball with the same contents as the folder). If you look inside that directory, you'll note that it doesn't contain a .git.

If you think about this, it makes sense, since if you publish a crate in some workspace, it's not like the whole workspace should be published.

BlackDex commented 2 years ago

@thomcc You are totally right. I didn't checked out what would be sent to crates.io and how that would work. The only way to have it working then would be to checkout the git repo it self via build.rs, and that would be an assumption that git would be installed at all, since that isn't mandatory, it then needs to be checked manually etc.. all to cumbersome and fault prone indeed.

I will change this PR (or close it) and just fix the CI part. Thanks for clarifying this!

BlackDex commented 2 years ago

Closing this in favor of #81 which only fixes the CI workflow, including fixes the extended.rs.