ezrosent / allocators-rs

Allocators in Rust
Apache License 2.0
311 stars 28 forks source link

Malloc bind unlikely #122

Closed serejkus closed 7 years ago

serejkus commented 7 years ago

This PR is a possible solution for #111. Actually, I hadn't found use cases for "likely", so I've put "unlikely" only.

I've put "unlikely" to "unhappy" paths, like NULL on free() and erroneous arguments in "posix_memalign". The motivation behind the second example is that the code not checking its arguments beforehand may be punished in terms of performance.

joshlf commented 7 years ago

Hey @serejkus, welcome and thanks for the PR!

This looks mostly good, although we'll have to wait for Travis to fix the issue it's having right now so we can run the integration tests.

One quick thing: can you rebase these into a single commit with a commit message long the lines of the following (see the CONTRIBUTING.md section on commit messages for details)?

malloc-bind: Use "unlikely" intrinsic

- Closes #111

Thanks!

joshlf commented 7 years ago

Is there a reason you didn't use unlikely on these lines?

serejkus commented 7 years ago

Yes, I didn't use unlikely in realloc since I've met this usage in cases like reallocation of vector: you can start with NULL pointer and grow it by calling realloc without special treatment for NULL. For example:

struct type_vector_t {
    type* data;
    size_t elems, capacity;
};

void init_type_vector_t(struct type_vector_t* vec) {
    vec->data = NULL;
    vec->elems = vec->capacity = 0;
}
void type_vector_append(type_vector_t* vec, type* elem) {
    if (elems == capacity) {
        type* new_data = realloc(vec->data, capacity * 2);
        // etc ....
    }
   // etc ....
}

I'll try to rebase my commits (sorry for not following contribution guide!), but I'm not sure whether github supports rewriting history in PR.

joshlf commented 7 years ago

OK, that makes sense. Probably a good idea not to use unlikely in that case.

As for rebasing, GitHub is fine with it - whatever you push to the branch (including force pushes that rewrite history) will be used for this PR.

joshlf commented 7 years ago

Looks like test failures are due to #120. The best bet is probably to wait until I merge a temporary fix in #125 and then rebase that.

joshlf commented 7 years ago

Can you rebase the new master now that the fix in #125 is merged?