-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Use system functions to allocate aligned memory #671
Conversation
This patch originated from #326. |
1146885
to
ad56fad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I am Ok with your approach. However, can you consider a set of proposed changes to reduce the code slicing and make macro dependent code more isolated inside the functions? What do you think? (I understand that it will cause one more call in the chain but I hope modern compilers can perform tail call optimization)
I'm quite certain the compiler won't optimize away the forwarding function, even if it amounts to a |
At rough estimation, I removed 6 condition blocks but added only 2.
It seems we have slightly different views here. The code is not on critical path in term of performance but any correctness issue should be avoided. However, code complexity wastes engineering time for debug and maintaining while it is not critical for performance of the whole library and we can spend this time to improve something else. As for optimization, it seems possible only for
I do not like to insist and prefer finding consensus. In any case, good balance is consist of multiple opinions. Taking into consideration that you contribute relatively often and reasonably, your opinion is valuable. |
Yes, it is better, I agree, but I just don't find the change that much better. I mean, the original code is not hard to decipher, and all conditions are uniform and easy to locate if they need to be updated. In my personal code I would probably be fine with the extra preprocessor checks if it makes the code more optimal.
Because MSVC is lucky enough for this optimization to be possible, and it seems wasteful to not take this opportunity.
The thing is, this project is not my personal code, and you, the maintainers, may have different priorities than I do, and this is totally fine. I'm just stating my position, and if you still think you prefer it the other way, I can change accordingly, and that is also fine. If I'm adamantly against something, you can be sure I will say so and refuse to change. :) In fact in this case I don't care about MSVC as my main target platform is Linux. I just added support for it because it was the straightforward and easy thing to do. My main goal here is to get this optimization merged, primarily for Linux, and I don't want this PR to hang indefinitely in limbo or get bogged down in discussions about minor things like this. So I will change the code as you suggest, no problem. |
Actually, it's not only MSVC - |
…memory. This uses the standard allocator for allocating and freeing memory with higher alignment requirements instead of the hand-rolled implementation. Signed-off-by: Andrey Semashev <[email protected]>
ad56fad
to
0365020
Compare
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution.
…memory. (#671) This uses the standard allocator for allocating and freeing memory with higher alignment requirements instead of the hand-rolled implementation. Signed-off-by: Andrey Semashev <[email protected]>
Description
This uses the standard allocator for allocating and freeing memory with higher alignment requirements instead of the hand-rolled implementation.
Type of change
Tests
Documentation
Breaks backward compatibility