Adding Rust Support #109
Comments
|
This is awesome! Firstly is there a specification of realloc aligned for Rust that I can read. There are a bunch of things that are done here to meet various C expectations that might not be required. The most efficient version will do no more than necessary. In particular, the interaction between alignment work and realloc work has many possibilities: What should size = 0, alignment = 16 do?
The order of checks is important to get each of these behaviours. Second minor point, if you reformat a file it makes code review very hard, if we are to take any changes they must be formated with our Clangformat config, our CI enforces this. |
|
Is it supposed to satisfy the following: https://doc.rust-lang.org/std/alloc/trait.GlobalAlloc.html#safety-4 How defensive do you want the code to be for undefined behaviour? The current code checks a lot of properties and returns error codes, but as the property failures are undefined behaviour we could be more efficient here? So it depends on the performance/usability trade-off for how this is implemented. I would suggest we turn a lot of stuff into asserts, so the Debug version checks everything and release is fast. |
|
I am so sorry that I am being terribly busy recently. I will back to this work at this weekend. :P |
|
@SchrodingerZhu no need to apologise, just let me know if there is anything I can do to help in advance. The project: mimalloc_rust looks like it could be good inspiration. It has fewer repos, then your current proposal, so probably easier to keep consistent. I think it makes sense to do the correctness checking for arguments in the Rust code as this will allow more inlining and better error reporting. I would propose you probably add a new file in That provides the API without any complex checking or assumptions that the C standard requires.
I would propose you call ThreadAlloc::get_noncachable() directly in your file. The (size,align) -> size calculation that I have factored out should be usable, but perhaps we need to move it to somewhere you can call it more easily. If you either copy or include malloc.cc for now, and I will refactor the function to somewhere else. You can call this may be faster than looking up in the page map the size of the allocation, but the calculation to align the value maybe slower. If there is anyway, we can cache or inline the Ultimately, knowing which bits cost is a case of getting data, so if we have profiling data we can see what needs improving. The aligned allocation path has not been heavily benchmark, so interested to see what you find. |
|
according to the document and the implementation of rust Layout, we can left many checking to the client. Hence, perhaps something like this is enough. BTW, will If the following code is okay, I will start merging current repos into one repo asap. #include "../mem/slowalloc.h"
#include "../snmalloc.h"
#include <cstring>
using namespace snmalloc;
inline size_t aligned_size(size_t alignment, size_t size)
{
// Client responsible for checking alignment is not zero
assert(alignment != 0);
// Client responsible for checking alignment is not above SUPERSLAB_SIZE
assert(alignment <= SUPERSLAB_SIZE);
// Client responsible for checking alignment is a power of two
assert(bits::next_pow2(alignment) == alignment);
size = bits::max(size, alignment);
snmalloc::sizeclass_t sc = size_to_sizeclass(size);
if (sc >= NUM_SIZECLASSES)
{
// large allocs are 16M aligned, which is maximum we guarantee
return size;
}
for (; sc < NUM_SIZECLASSES; sc++)
{
size = sizeclass_to_size(sc);
if ((size & (~size + 1)) >= alignment)
{
return size;
}
}
// Give max alignment.
return SUPERSLAB_SIZE;
}
extern "C" void* rust_alloc(size_t alignment, size_t size) {
return ThreadAlloc::get_noncachable()->alloc(aligned_size(alignment, size));
}
extern "C" void rust_dealloc(void* ptr, size_t alignment, size_t size) {
ThreadAlloc::get_noncachable()->dealloc(ptr, aligned_size(alignment, size));
}
extern "C" void* rust_realloc(void* ptr, size_t alignment, size_t size, size_t new_size) {
size_t old_size = aligned_size(alignment, size);
new_size = aligned_size(alignment, new_size);
if (new_size == old_size) return ptr;
void* p = ThreadAlloc::get_noncachable()->alloc(new_size);
if (p) {
std::memcpy(p, ptr, old_size < new_size ? old_size : new_size);
ThreadAlloc::get_noncachable()->dealloc(ptr, old_size);
}
return p;
}Another thing to be considered is that (as you have mentioned), will the re-calculation of aligned memory (in dealloc and realloc) leads to a performance issue? |
|
So I think your implementation would work. I have one question that isn't clear from the standard, but I think you are doing the right thing:
I think as it says the outer Rust call, must be the same Layout as originally called with, thus I think it cannot change. However, there appears to be a gap in the specification as it doesn't specify what to consider the Layout for an allocation returned by The other thing is, I think you are extern "C" void* rust_realloc(void* ptr, size_t alignment, size_t old_size, size_t new_size) {
size_t align_old_size = aligned_size(alignment, old_size);
align_new_size = aligned_size(alignment, new_size);
if (align_new_size == align_old_size) return ptr;
void* p = ThreadAlloc::get_noncachable()->alloc(align_new_size);
if (p) {
std::memcpy(p, ptr, size < new_size ? old_size : new_size);
ThreadAlloc::get_noncachable()->dealloc(ptr, align_old_size);
}
return p;
}Given the current API there is no way to know how |
|
A published version of However, the documentation failed to build (because of Any suggestion? |
|
Could you "fake" snmalloc when building for docs.rs? Don't build or link to snmalloc, and Or change the |
|
I think it is more reasonable not to set |
|
If you want some publicity about this, let me know, and I can tweet about it. There is currently some buzz about Verona, which is why we wrote the allocator in the first place. So can probably ride that wave. Let's just be sure it is working well before we draw attention to it. :-) If you want me to tweet about it, and you have a twitter handle, can you let me know it. |
|
The packages are ready at crates. There are two more things, and I will try to check them in the following days:
|
|
I couldn't make it work in Windows with MSVC and will try spending more time on this, but meanwhile some feedback:
Versions:
|
|
Thanks @snf for taking a look. I have pushed a branch, For CMake, if you specify, '-Ax64', then it will build the 64bit version for which ever version of Visual Studio you have (I think). But you can't set this for Makefiles or Ninja. |
|
My observation is that Something like the attached build script should fix the linking problem. However, I sadly run into a new problem: The use cmake::Config;
fn main() {
let mut cfg = &mut Config::new("snmalloc");
let build_type = if cfg!(feature = "debug") {
"Debug"
} else {
"Release"
};
cfg = cfg.define("SNMALLOC_RUST_SUPPORT", "ON")
.profile(build_type);
let target = if cfg!(feature = "1mib") {
"snmallocshim-1mib-rust"
} else {
"snmallocshim-rust"
};
let mut dst = if cfg!(feature = "cache-friendly") {
cfg.define("CACHE_FRIENDLY_OFFSET", "64").build_target(target).build()
} else {
cfg.build_target(target).build()
};
dst.push("./build");
println!("cargo:rustc-link-lib={}", target);
if cfg!(unix) {
println!("cargo:rustc-link-search=native={}", dst.display());
println!("cargo:rustc-link-lib=dylib=stdc++");
println!("cargo:rustc-link-lib=dylib=atomic");
} else {
println!("cargo:rustc-link-search=native={}/{}", dst.display(), build_type);
}
} |
|
You need # VirtualAlloc2 is exposed by mincore.lib, not Kernel32.lib (as the
# documentation says)
target_link_libraries(snmalloc_lib INTERFACE mincore)Not sure why it isn't. |
|
A CI is added for this： |
|
@mjp41 I do not have a twitter account but it will be great if you can let more people test on these rust crates. Since my benchmark project still has a long way to go it will be useful if others who are interested do more experiments using their existed projects. I actually did very little work -- just some bindings and adjustments on compilation. The crates cannot exist without your great research. I just copied some sentences in this repo to create the current document of the rust library. If you have checked the document and no issue has been found, I think #109 is ready to be closed. |
|
@snf and @plietar could you confirm this works for some Rust projects? Could we build a Rust project with a rustc using snmalloc? @SchrodingerZhu I'll submit a PR to your repo with some minor documentation fixes. Repo: https://github.com/SchrodingerZhu/snmalloc-rs |
|
I checked one of my private projects with it and it worked so I guess there is nothing super wrong with it :). I left running some benchmarks in LocustDB and will report back tomorrow. |
|
All looks good, so closing this issue. Thanks @snf and @SchrodingerZhu for the hard work. Perf numbers look pretty good too. |
I am trying to bind
snmallocto rust to have a comprehensivemalloc-bench(WIP).Basically, I have already created several bindings:
snmalloc,snmalloc-sys,snmallocator.Yet, there is a issue to provide efficient
reallocwith alignment. I added one following the code inmalloc.h, but I do not know whether it is sound and safe. So I guess I need some guidance here.The text was updated successfully, but these errors were encountered: