SFBdragon / talc

A fast and flexible allocator for no_std and WebAssembly
https://crates.io/crates/talc
MIT License
419 stars 9 forks source link

Improve global allocator documentation #18

Closed polarathene closed 11 months ago

polarathene commented 12 months ago

The current example:

I was curious how talc might compare to mimalloc:

use mimalloc::MiMalloc;

#[global_allocator]
static GLOBAL: MiMalloc = MiMalloc;

But perhaps they serve different audiences 😅


EDIT: I compared the nightly toolchain (with this example test that shows degraded perf with multi-thread on musl) between talc and mimalloc and while they were similar on single thread ( ~300ms), multi-thread had a notable difference (10 threads):

I noticed talc emphasizing single-thread usage, and I was curious how it compared in a multi-thread context. I guess the talc allocator spin mutex would have been blocking for multi-thread to slow it down to ~2.5x.

SFBdragon commented 12 months ago

Hi!

  1. Yep, it does. Talc doesn't prescribe the use of any particular mutex (using lock_api to be fairly agnostic), but that of spin is a sensible choice. I have options here: A) use the unsafe AssumeUnlockable type that comes with my crate (mainly for WASM), but runs the risk of people copy and pasting code into contexts where this is not a correct assumption and causes heisenbugs. B) keeping it as is (but perhaps adding a comment to the example to clarify the external dependency?) C) a funky and fresh option that I have not considered

  2. I'll make a stable example alternative, thanks for the suggestion.

Regarding MiMalloc, This allocator's niche is outside of MiMalloc's, they aren't competitors, but instead should be used where the other shouldn't/can't. (MiMalloc is strictly for hosted systems, as far as I can tell.)

I briefly looked into trying to implement platform-specific support for fast SMP allocation a while back, but I came away with the impression that this allocator hasn't been designed from the ground up in a way that would make it a competitive choice for multithreaded applications. Doing so would be a lot of work, only to enter a far more competitive niche.

If you're developing an application that falls within its strengths, please use it instead. It looks very impressive.

If, for example, you're interested in OSdev and getting a project like Talc to efficiently integrate with your own threading system, besides keeping an allocator per-CPU, I'm interested in working on perhaps better-integrating something like this, so please open an issue if you're interested in more support from Talc for something like that.

SFBdragon commented 12 months ago

Regarding 2, this example should work without nightly, as it's not using the nightly-gated functions (which is just the *mut [T] -> Span conversions due to https://github.com/rust-lang/rust/issues/71146 or https://github.com/rust-lang/rust/issues/81513)

I believe I should clarify this in the Stable Rust section. Burying this in the changelog isn't ideal.

polarathene commented 12 months ago

B) keeping it as is (but perhaps adding a comment to the example to clarify the external dependency?)

That might be helpful? 👍

I don't have a need for talc any time soon, but when I came across it I did want to try it out, so just followed the advice I saw on the README and tried the example.

Wasn't particularly difficult to realize from the failure that I needed to bring in the dep:

error[E0433]: failed to resolve: use of undeclared crate or module `spin`
  --> src/main.rs:11:25
   |
11 | static ALLOCATOR: Talck<spin::Mutex<()>, ClaimOnOom> =
   |                         ^^^^ use of undeclared crate or module `spin`

cargo add spin works fine, I wasn't sure if it needed to be a bit more explicit like the Cargo.toml line I referenced earlier with:

default-features = false, features = ["lock_api", "spin_mutex"]

Regarding 2, this example should work without nightly

I'm probably doing something wrong then?

[dependencies]
talc = { version = "3.1.1", default-features = false, features = ["lock_api"] }
spin = { version = "0.9.8", default-features = false, features = ["lock_api", "spin_mutex"] }
$ cargo --version
cargo 1.74.0 (ecb9851af 2023-10-18)

$ cargo run
   Compiling hello_world v0.1.0 (/tmp/hello_world)
error[E0554]: `#![feature]` may not be used on the stable release channel
 --> src/main.rs:1:1
  |
1 | #![feature(const_mut_refs)]

# Removed feature attribute
$ cargo run
   Compiling hello_world v0.1.0 (/tmp/hello_world)
error[E0658]: mutable references are not allowed in statics
  --> src/main.rs:12:57
   |
12 |     Talc::new(unsafe { ClaimOnOom::new(Span::from_array(&mut ARENA)) }).lock();
   |                                                         ^^^^^^^^^^
SFBdragon commented 12 months ago

I'll go ahead with adding a clarifying comment for the spin dependency.

I'm probably doing something wrong then?

You are absolutely correct, my bad!

Here's my workaround. Indeed it's nonobvious. Here's what I'll update the example to:

 Span::from_base_size(&ARENA as *const _ as *mut _, 10000)
 // Span::from_array(&mut ARENA) - better but requires unstable #[feature(const_mut_refs)]

It's not pretty but it's the best I know of. (Unfortunately addr_of_mut!(ARENA) also requires const_mut_refs for some reason).

I'll close this once I publish these changes to the project, probably quite soon. Thanks for reaching out!

SFBdragon commented 11 months ago

After more delay than it should've been, the changes are in and a new minor version has been released. Thanks once again.