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

Refactor package, separating byte calculations out #3

Merged
merged 50 commits into from
Apr 29, 2022
Merged

Conversation

mkitti
Copy link
Owner

@mkitti mkitti commented Apr 8, 2022

I have done a major refactoring of this package on the total_refactor branch: https://github.com/mkitti/ArrayAllocators.jl/tree/total_refactor

@mkitti
Copy link
Owner Author

mkitti commented Apr 13, 2022

@bjarthur and @carstenbauer , I would appreciate if you could review the refactored design.

@mkitti
Copy link
Owner Author

mkitti commented Apr 13, 2022

A documentation preview can be found here:
https://mkitti.github.io/ArrayAllocators.jl/previews/PR3/

@carstenbauer
Copy link

carstenbauer commented Apr 19, 2022

A few comments:

  • On https://mkitti.github.io/ArrayAllocators.jl/previews/PR3/bytecalculators/, the index list mentions NUMA stuff but the actual functions aren't on that page (but in the NUMA section). Same issue here
  • numa and NumaAllocator are not available. I get UndefVarError when I try to run the examples in the docs. And using NumaAllocators, as written in the README.md, also doesn't work. How does that work? (And it should be documented.)
  • MemAlign isn't mentioned in the docs? BTW, for testing that the alignment is actually correct, you can get inspiration here.
  • IMO, you should cleanly separate examples from API references / interfaces, i.e. avoid a "everything on one page" layout like here. See https://documentation.divio.com/

@mkitti
Copy link
Owner Author

mkitti commented Apr 19, 2022

Thanks for the review @carstenbauer. Yes, I am primarily trying to improve the documentation now. I think part of the issue is that I need to move some of the docstrings outside of the operating system specific if statements.

  • numa and NumaAllocator are not available. I get UndefVarError when I try to run the examples in the docs. And using NumaAllocators, as written in the README.md, also doesn't work. How does that work? (And it should be documented.)

Eventually, using NumaAllocators should work when I register the packages. For now, the following should work:

] activate --temp
] add https://github.com/mkitti/ArrayAllocators.jl#total_refactor
] add https://github.com/mkitti/ArrayAllocators.jl#total_refactor:NumaAllocators
] add https://github.com/mkitti/ArrayAllocators.jl#total_refactor:SafeByteCalculators

@mkitti
Copy link
Owner Author

mkitti commented Apr 20, 2022

@carstenbauer, what happens when T, the element type, is not a bitstype?

@carstenbauer
Copy link

@carstenbauer, what happens when T, the element type, is not a bitstype?

For MemAlign? I would think that it just doesn't work / errors.

@mkitti
Copy link
Owner Author

mkitti commented Apr 20, 2022

We currently throw an error for PosixMemAlign. I'm wondering if we should throw an error more generally for other allocators other than PosixMemAlign.

@mkitti
Copy link
Owner Author

mkitti commented Apr 29, 2022

Thank you for the reviews, @carstenbauer and @bjarthur. I'm going to merge and release this now.

@mkitti mkitti merged commit 447be2b into main Apr 29, 2022
@mkitti mkitti mentioned this pull request Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants