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

Avoid assertion messages in free_list_allocator in release builds. #7

Merged
merged 4 commits into from
Feb 24, 2024

Conversation

sunfishcode
Copy link
Contributor

Change some asserts to debug_asserts, for smaller code size in release builds.

And, avoid doing an integer divison in round_up and multiple_below, by taking advantage of the fact that the denominator is always a power of two. These functions are often inlined, in which case the optimizer can see the powers of two and do this optimization itself, however this isn't always done when optimizing for size.

Change some `assert`s to `debug_assert`s, for smaller code size in
release builds.

And, avoid doing an integer divison in `round_up` and `multiple_below`,
by taking advantage of the fact that the denominator is always a power
of two. These functions are often inlined, in which case the optimizer
can see the powers of two and do this optimization itself, however this
isn't always done when optimizing for size.
Comment on lines 173 to 174
// From https://github.com/wackywendell/basicalloc/blob/0ad35d6308f70996f5a29b75381917f4cbfd9aef/src/allocators.rs
// Round up value to the nearest multiple of increment
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to document the new requirement on increment here.

And while you are at it, a doc comment on multiple_below for it as well would be good.

@Craig-Macomber
Copy link
Owner

Somehow I forgot about debug_assert when I wrote this, and that absolutely is what I intended. Great fix. This rest is probably good, but could use a bit more docs as I noted with inline comments.

Thanks for the change: If you don't get around to addressing my feedback on this PR, I'll do so myself later when I have more time as this is a great improvement which I do want to get merged.

@sunfishcode
Copy link
Contributor Author

I've now added comments and a round_up unit test.

@Craig-Macomber
Copy link
Owner

I checked (using test.sh) and this seems to save 1 byte in my test case. I suspect apps with more dynamic allocation calls will benefit more, but its nice to confirm its a win even in my test.

@Craig-Macomber Craig-Macomber merged commit bde5e16 into Craig-Macomber:main Feb 24, 2024
1 check passed
@Craig-Macomber
Copy link
Owner

Published in 0.4.1

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

Successfully merging this pull request may close these issues.

None yet

2 participants