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

DirectAllocator alignments > os.page_size on posix #939

Merged
merged 1 commit into from Apr 22, 2018

Conversation

Projects
None yet
3 participants
@tgschultz
Contributor

tgschultz commented Apr 22, 2018

Added support to DirectAllocator for alignments larger than the page size on posix. This is implemented by using mmap to acquire enough address space to ensure we can align the specified size, then calling munmap on any unused pages.

if(alloc_size == n) return @intToPtr(&u8, addr)[0..n];
var aligned_addr = addr & ~usize(alignment - 1);

This comment has been minimized.

@bnoordhuis

bnoordhuis Apr 22, 2018

Member

What does ~usize(alignment - 1) compute when alignment == 0? 0x1fffffff? That'd do the wrong thing when &-ed with the address. What about alignments that aren't a power of two?

This comment has been minimized.

@tgschultz

tgschultz Apr 22, 2018

Contributor

Are non-po2 alignments supported? The current implementation of DirectAllocator for posix assumes po2, and I have never heard of a use case for anything other than po2 alignment.

That said, the Windows implementation allows it. If it is not supported, then we should assert alignment is po2, but for now I'll take a look at supporting it.

Align of 0 is < os.page_size and therefore would not trigger this code.

This comment has been minimized.

@andrewrk

andrewrk Apr 22, 2018

Member

I'll add this documentation to std.mem.Allocator:

  • alignment is guaranteed to be >= 1
  • alignment is guaranteed to be a power of 2

This comment has been minimized.

@andrewrk

andrewrk Apr 22, 2018

Member

This is actually enforced by the compiler:

        return ([]align(alignment) T)(@alignCast(alignment, byte_slice));

if you passed 0 or non power of 2 to alignment the cast in this line will give you a compile error.

// pass munmap bytes that exist outside our allocated pages or it
// will happily eat us too
//Since alignment > page_size, we are by definition on a page boundry

This comment has been minimized.

@bnoordhuis

bnoordhuis Apr 22, 2018

Member

Typo: boundary

//It is impossible that there is an unoccupied page at the top of our
// mmap.
return @intToPtr(&u8, aligned_addr)[0..n];

This comment has been minimized.

@bnoordhuis

bnoordhuis Apr 22, 2018

Member

Correct me if I'm wrong but doesn't this leak the first pages when the result is passed to .free()?

This comment has been minimized.

@tgschultz

tgschultz Apr 22, 2018

Contributor

Any pages we'd mapped that are below our new alignment address are unmapped already not 6 lines prior. As far as I can tell this is working as expected.

fn testAllocatorLargeAlignment(allocator: &mem.Allocator) mem.Allocator.Error!void {
//Maybe a platform's page_size is actually the same as or
// very near usize?
if(os.page_size << 2 > @maxValue(usize)) return;

This comment has been minimized.

@bnoordhuis

bnoordhuis Apr 22, 2018

Member

Isn't this logically always false if os.page_size == @maxValue(usize)? As well, this check is the kind of hypothetical I wouldn't worry about until it becomes an actual problem.

This comment has been minimized.

@tgschultz

tgschultz Apr 22, 2018

Contributor

Since everything involved is a literal, I don't believe there is any overflow to worry about. However, @MaxValue(usize) really should be (1 << usize.bit_count). Of course this all assumes po2 page size, but I don't think that's worth worrying about until LLVM supports a ternary native architecture.

@andrewrk andrewrk merged commit 3010668 into ziglang:master Apr 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@andrewrk

This comment has been minimized.

Member

andrewrk commented Apr 22, 2018

Thanks! Out of curiosity, when do you need an alignment greater than page size?

@tgschultz tgschultz deleted the tgschultz:large-alignment-directalloc branch Apr 22, 2018

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