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 over-allocatng memory due to alignment constraints #509

Closed
romange opened this issue May 9, 2024 · 3 comments · Fixed by #510
Closed

avoid over-allocatng memory due to alignment constraints #509

romange opened this issue May 9, 2024 · 3 comments · Fixed by #510

Comments

@romange
Copy link
Contributor

romange commented May 9, 2024

in heap_string_factory::create we over-allocate memory to be able to align the pointer withing the allocated block.

However, the default, usual case is that we allocate strings with alignment 8 (= alignof(storage_type).
But all the standard allocators available today in any os already return 8 byte-aligned pointers, so overallocating by 7 bytes is unnecessary. One can think that 7 bytes is not significant but it could push the underlying allocator to select the next size class for the allocation, effectively reserving dozens of bytes more.

To summarize: standard allocators already return 8 bytes aligned pointers, so we pay penalty for supporting the theoretic edge cases that return non-aligned adresses.

My suggestion is that for alignments <= 8 we wont allocate exacly the requested size and check if the returned address is aligned.
And if it hits the unlikely scenario of not being aligned, then deallocate and retry again with the current approach.

@danielaparker
Copy link
Owner

Thanks for the suggestion. It seems the pmr allocators don't return maximally aligned pointers, which is where the issue first arose, see (#441), and the reason we added the alignment code. I agree that for most of our users it's wasted space.

@romange
Copy link
Contributor Author

romange commented May 9, 2024

Yes, indeed for alignments > 8, the original code did not work, as @chakaz pointed out. Would you mind if I send the fix that solves it for both edge cases?

@danielaparker
Copy link
Owner

@romange Feel free to submit a PR

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

Successfully merging a pull request may close this issue.

2 participants