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

mallocator: align allocations to 64 bytes #282

Open
lidavidm opened this issue Feb 17, 2025 · 12 comments · May be fixed by #289
Open

mallocator: align allocations to 64 bytes #282

lidavidm opened this issue Feb 17, 2025 · 12 comments · May be fixed by #289
Assignees
Labels
Type: enhancement New feature or request

Comments

@lidavidm
Copy link
Member

Describe the enhancement requested

Some types want 16 byte (or more) alignment, such as decimal256, and it's easiest just to overallocate + align to avoid issues (even if it's not strictly required by the spec, many things in practice assume it). We have to overallocate since the allocator API is not given an alignment.

Context: apache/arrow-adbc#2526

Component(s)

Other

@lidavidm
Copy link
Member Author

lidavidm commented Feb 17, 2025

Ah, but Go directly uses []byte, so we have nowhere to stash the "real" pointer...

@lidavidm
Copy link
Member Author

The best thing I can think of is to wrap aligned_alloc but that carries a number of caveats (requires C11, not portable to Windows, does not support realloc, cannot align an allocation smaller than the alignment). Maybe we can consider this if there's enough demand.

@zeroshade
Copy link
Member

We're still using a memory. Buffer and can use slices, so it wouldn't be difficult to modify the buffer struct to stash the "real" pointer. We stash the pointer and then have the slice start at the aligned data?

@lidavidm
Copy link
Member Author

I was looking at that. In that case maybe we just make Buffer handle the alignment and leave the allocator out of it?

@zeroshade
Copy link
Member

Possibly, we'd have to make sure that the release callback still releases the original pointer, but otherwise it could work if we make sure to overallocate.

@lidavidm
Copy link
Member Author

Well otherwise we'd have to change the allocator API to return the (base, aligned) pair

@zeroshade
Copy link
Member

That's a good point! It's definitely easier to manage in the buffer class and use the slice!

@lidavidm
Copy link
Member Author

The other complication is resizing, since resizing won't preserve the alignment; we may have to copy

@zeroshade
Copy link
Member

Resizing frequently requires a copy anyways, so as long as we handle it properly it should be okay I think?

@lidavidm
Copy link
Member Author

Yeah.

The other thing is the native Go allocator already does the trick, so we need to take care not to double-do it. So I'm thinking I'll either add a new "AllocateAligned" that does return the pair, or a flag on the allocator interface to indicate whether the allocator handles it natively or not.

@zeroshade
Copy link
Member

Go has an AlignOf method that might help?

@lidavidm
Copy link
Member Author

Well then we'd have to allocate and try again?

I'm just saying that, if the allocator already does the padding trick underneath (or otherwise somehow supports aligning the allocation), then we shouldn't add the padding ourselves in Buffer

lidavidm added a commit to lidavidm/arrow-go that referenced this issue Feb 18, 2025
@lidavidm lidavidm linked a pull request Feb 18, 2025 that will close this issue
lidavidm added a commit to lidavidm/arrow-go that referenced this issue Feb 21, 2025
lidavidm added a commit to lidavidm/arrow-go that referenced this issue Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants