Skip to content

Conversation

@thomcc
Copy link
Contributor

@thomcc thomcc commented Jul 5, 2020

Fixes #31. Comes with tests, more CI, and docs.

This goes on top of #30 since it improves the CI. It also includes the fix for #29 which is a one-liner and is required to make the tests pass.

Once #30 that lands I'll rebase and the commits from it should go away, and if you like the look at this I can close #29 and just let it get subsumed.

This splits stuff into an "extended" API (activated by a feature) and a default API. This is a bit odd, since it's not really on a good/deliberate boundary.

I think perhaps a better boundary would be:

  • The minimal API is just the things needed by the mimalloc::Mimalloc implementation
  • The extended API is everything else.

As it is it's kind of weird that it includes mi_zalloc or mi_usable_size in the "core", but mi_calloc is part of the extended api. That said, the code is probably fine as-is -- it would be breaking to move public stuff behind a feature, and really there's little benefit to it, so it seems fine, just odd.

What this skips:

I'm not that attached to most of the things on this list, and would be willing to add them either now or in the future as-needed.

Anyway, the following parts of the API are intentionally not exposed because they

  • The C++ wrappers as they're just the normal mimalloc API but with throwing C++ exceptions on failure, which is probably unsound.

  • The Posix functions as they're all aliases or tiny wrappers for the normal mimalloc API.

  • All of these not captured by Posix/C++ (above).

  • The Typed Macros for obvious reasons.

  • The Zero initialized re-allocation functions as they're easy to misuse (only valid on pointers coming from a zero-init alloc function in the first place).

  • Anything labeled experimental, which is:

  • Anything deprecated, e.g. mi_reserve_huge_os_pages.

  • I'm also omitting a few other functions that are not very useful and have potential to cause problems:

    • mi_option_enable/mi_option_disable: Just a wrapper for mi_option_set_enabled, and had a different argument count in the past

      In practice this seems unlikely to matter, but given that they're just wrappers, no reason to risk it in case this crate allows linking to an external mimalloc in the future, and we get an older one.

I think that's it.


Looking at it again, zero initialized re-allocation is probably worth adding -- it's a low level API and that can help. It's easy to misuse, but that's why it's unsafe.

Hrm.

@thomcc
Copy link
Contributor Author

thomcc commented Jul 12, 2020

Gentle ping :)

@octavonce
Copy link
Collaborator

@thomcc Sorry for taking so long. I have had the busiest times of my life in the past month and I wanted to take the time to review these properly.

#29 and #30 are now merged. I believe a rebase from you should do the trick for this one. Very well done with these changes!

@octavonce
Copy link
Collaborator

This looks great! Merging now...

@octavonce octavonce merged commit 599a628 into purpleprotocol:master Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants