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.

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

Improve global allocator documentation #18

Closed
polarathene opened this issue Nov 18, 2023 · 5 comments
Closed

Improve global allocator documentation #18

polarathene opened this issue Nov 18, 2023 · 5 comments

Comments

@polarathene
Copy link
Contributor

polarathene commented Nov 18, 2023

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):

  • mimalloc: ~45ms (musl is ~330ms without mimalloc)
  • talc: ~800ms

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
Copy link
Owner

SFBdragon commented Nov 19, 2023

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
Copy link
Owner

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 rust-lang/rust#71146 or rust-lang/rust#81513)

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

@polarathene
Copy link
Contributor Author

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
Copy link
Owner

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 added a commit that referenced this issue Nov 30, 2023
@SFBdragon
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants