Refactor package, separating byte calculations out

mkitti / ArrayAllocators.jl

Allocate arrays with malloc, calloc, or on NUMA nodes

MIT License

53 stars 1 forks source link

Refactor package, separating byte calculations out #3

Closed mkitti closed 2 years ago

mkitti commented 2 years ago

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

mkitti commented 2 years ago

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

mkitti commented 2 years ago

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

carstenbauer commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

carstenbauer commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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