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

ARC now capable of custom extra alignment. Ref, closure and seq support. #15697

Merged
merged 27 commits into from
Oct 28, 2020

Conversation

cooldome
Copy link
Member

@cooldome cooldome commented Oct 23, 2020

Max Alignment is 256 bytes. Let me know if you want me to increase it.
Max Alignment is 32768 bytes.

For ref T I had to do the following trick, suppose the alignment is 64 bytes and we are overallocating then memory layout is: padding first then RefHeader then the object. Hence there is a padding at start such that head(myref) stil works and it does cast[int](myref) - sizeof(RefHeader).

New functions alignedAlloc, alignedRealloc, alignedDealloc mimic what the functions posix_memalign, aligned_alloc do but in the platform independent way. These functions are overallocating on windows and linux.

@cooldome cooldome changed the title ARC now capable of custon large alignment. Ref, closure and seq support. ARC now capable of custom extra alignment. Ref, closure and seq support. Oct 23, 2020
@cooldome
Copy link
Member Author

ready for review, test failures seem unrelated

@cooldome
Copy link
Member Author

found the bug with useMalloc. Investigating

@cooldome
Copy link
Member Author

cooldome commented Oct 26, 2020

Ready for review. Increased max alignment to 32768.

@Araq
Copy link
Member

Araq commented Oct 27, 2020

New functions alignedAlloc, alignedRealloc, alignedDealloc mimic what the functions posix_memalign, aligned_alloc do but in the platform independent way. These functions are overallocating on windows and linux.

Does this mean that we also overallocate with our default native Nim allocator? When exactly do we overallocate?

@cooldome
Copy link
Member Author

cooldome commented Oct 27, 2020

Here are my findings. Functions like posix_memalign, aligned_alloc are always overallocating because they need to store alignment offset somewhere hence using them in all cases is not an option as Nim's memory footprint would increase significantly. There is no standard way to reallocate aligned memory while this op is used by seqv2 implementation. Hence I implemented my own portable
version of these functions.

In order to keep overhead low I am checking if extra alignment is required at all. If not I revert back to standard allocation (either nim allocator or c_malloc depending on useMalloc).
The downside is that alignment needs to be passed as the argument in all cases even for dealloc. Such that if statement could be done:

 proc alignedDealloc(p: pointer, align: int) {.compilerproc.} = 
    if align <= MemAlign:
      existingDealloc(p)
    else:      
      let offset = cast[ptr uint16](p -! sizeof(uint16))[]
      existingDealloc(p -! offset)

Summary:
Alignment works both with useMalloc and Nim allocator. For types that doesn't need extra alignment the only overhead is extra if statement if align <= MemAlign:. Compared to allocation itself is a small overhead.

@cooldome
Copy link
Member Author

cooldome commented Oct 27, 2020

Also it is a good time to reduce default MemAlign to reduce memory footprint for apps.

@cooldome
Copy link
Member Author

fixes #7865

result = alloc(size)
else:
when compileOption("threads"):
let base = allocShared(size + align - 1 + sizeof(uint16))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Write a comment that explains this logic. And also why it doesn't produce unaligned stores (which can be performance desasters on non-x86 CPUs).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added the comments. I have carefully reviewed the code for unaligned reads or stores. I dont' see any.

@cooldome
Copy link
Member Author

ready for review

@Araq Araq added the merge_when_passes_CI mergeable once green label Oct 28, 2020
@narimiran narimiran merged commit 0956a99 into devel Oct 28, 2020
@Araq Araq deleted the aligned_alloc_for_use_malloc branch October 28, 2020 13:01
@planetis-m
Copy link
Contributor

planetis-m commented Oct 28, 2020

@cooldome Do you plan to make doc comments, export aligned* and add changelog entry in a follow up or it wasn't exported on purpose?

narimiran pushed a commit that referenced this pull request Nov 5, 2020
mildred pushed a commit to mildred/Nim that referenced this pull request Jan 11, 2021
irdassis pushed a commit to irdassis/Nim that referenced this pull request Mar 16, 2021
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge_when_passes_CI mergeable once green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants