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

Fix aligned memory allocation #1193

Merged
merged 7 commits into from
May 21, 2021
Merged

Fix aligned memory allocation #1193

merged 7 commits into from
May 21, 2021

Conversation

Smertig
Copy link
Contributor

@Smertig Smertig commented May 10, 2021

Closes #1192

  • Reworked sol::detail::aligned_space_for<Ts...>
    • If no type is over-aligned (alignof(Ts) <= alignof(std::max_align_t) for all Ts...), calculate maximum possible required size assuming that allocator can return absolutely any address
    • Otherwise assume that every single type in pack has initial address 0x1 (the worst alignment)
    • Make it constexpr
  • Always do only one allocation with required memory size. "aligned allocation failed" error should never happen anymore
  • Remove unused arguments/variables (it was probably a legacy code)

@ThePhD
Copy link
Owner

ThePhD commented May 16, 2021

This actually does look good to me, albeit the tests will always fail for one or two platforms because we're running into a hard regression thanks to lua_error thrashing the stack in some cases of optimized code. Can't escape it, so the CI will always go red.

I'm gonna run this locally and try to fix things up, so forgive me if merging this takes a little bit!!!

@ThePhD ThePhD self-assigned this May 21, 2021
@ThePhD ThePhD added the Bug.Derp They don't call me The Phantom Derpstorm for no reason. label May 21, 2021
@ThePhD
Copy link
Owner

ThePhD commented May 21, 2021

Lllllooooooooooooooooooooooks good to me and makes things much faster / more consistent when alignment gets weird. Thanks so much for this!

I was originally going to promote these functions out of the detail namespace since they're used but I'll do that later, since that's technically a breaking change.

@ThePhD ThePhD closed this May 21, 2021
@ThePhD ThePhD reopened this May 21, 2021
@ThePhD ThePhD merged commit bb5f60e into ThePhD:develop May 21, 2021
@ThePhD
Copy link
Owner

ThePhD commented May 21, 2021

Ssssskskskskdskdjakdasjk I closed it and almost panicked because the merge button disappeared

I AM A PROFESSIONAL I PROMISE!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug.Derp They don't call me The Phantom Derpstorm for no reason.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Aligned allocation of userdata block (data section) for Test failed
2 participants