Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added mimalloc dev and dev-slice support #75

Closed
wants to merge 2 commits into from
Closed

Added mimalloc dev and dev-slice support #75

wants to merge 2 commits into from

Conversation

BlackDex
Copy link
Contributor

@BlackDex BlackDex commented Jan 16, 2022 •
edited

  • Added support to use the v1 dev branch or v2 dev-slice branch
  • Added option to update the git submodules to be able to use the latest commits from upstream mimalloc.

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 microsoft/mimalloc#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
Copy link
Contributor Author

I fixed the cargo fmt error.

@BlackDex
Copy link
Contributor Author

@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
Added mimalloc dev and dev-slice support
5a33939 
- Added support to use the v1 dev branch or v2 dev-slice branch
- Added option to update the git submodules to be able to use the latest commits from upstream mimalloc.
@BlackDex
Fixed CI testing
26fc715 
This commit should solve the CI errors.

- Using `ctest2` instead of `ctest` which has not been updated for a while
- Fixed extended feature because `mi_option_reserve_huge_os_pages_at` was added
@BlackDex
Copy link
Contributor Author

@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
Copy link
Contributor Author

@octavonce any feedback on this pr??

@happypsyduck
Copy link

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

@BlackDex
Copy link
Contributor Author

BlackDex commented Jun 4, 2022

@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
Copy link
Contributor

thomcc commented Jun 10, 2022 •
edited

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.

thomcc
thomcc suggested changes Jun 10, 2022
View reviewed changes
Copy link
Contributor

@thomcc thomcc left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The submodule_update parts of this don't work, as mentioned. I think they should be removed, since there's no way to make them work either.

I also think that having dev/dev-slice branches has much less value given this, since it's still tied to the versions that exist when when published.

Also, we now use 2.x anyway, so I don't really see the point.

@BlackDex
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@BlackDex BlackDex closed this Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomcc thomcc thomcc requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update submodule from v1.6.7 to v2.0.0
3 participants
@BlackDex @happypsyduck @thomcc