Comments on SegmentAllocator and ResourceScope

[Maurizio Cimadamore]

On 17/05/2021 07:47, Michael Zucchi wrote:
> Looks a bit odd but note that the original segment is still the 
> current one for subsequent allocations so it will still be used for 
> other small allocations, it probably doesn't have much effect but it 
> might reduce allocation holes.  Also whatever the value is the limit 
> has to be half the block allocation size for alignment to work 
> properly since the base allocations are assumed to be unaligned.
>
> It's also testing against the bytesAlignment aligned size which is not 
> the value bytesAlignment applies to, but by having MAX_ALLOC_SIZE == 
> BLOCK_SIZE/2 this also 'magically' tests the unaligned allocation size 
> limit required to guarantee satisfaction of the alignment as well.  
> Seems overly tricksy.  Also not a fan of "this should not be possible" 
> comment & assertion that only /seems/ to be there to check this 
> tricksiness and enforce the internal expected behaviour of trySlice() 
> - which isn't really serving any purpose as a separate function since 
> the 'try' only affects the first invocation and it isn't used elsewhere.
>
> This also causes the bug above and seems to indicate the 
> straightforward logic flow of "try-to-fit ELSE IF SMALL 
> allocate-and-slice ELSE allocate-separate" all in a single function 
> would be better - even if it does end up with more holes in certain 
> cases (you win some, you lose some).
It's true that MAX_ALLOC_SIZE is set to half the block so that we can 
still adjust for unaligned allocations within the allocated block. I'll 
see if the logic can be simplified - at the very minimum, if we want to 
reuse implementation between bounded and unbounded, some of these limits 
(block size, max allocation size) need to be made instance-dependent.
>
> As an aside the javadoc doesn't seem mention anywhere that 
> bytesAlignment must be a power of 2.  Neither is it checked for 
> validity, that may be on purpose for performance reasons but the 
> comparable posix_memalign() /aligned_alloc() fail with EINVAL if it 
> isn't and well things can get pretty bizarre if it isn't valid.  It 
> isn't an expensive check, 
> https://graphics.stanford.edu/~seander/bithacks.html#DetermineIfPowerOf2 
> or equivalently (Long.lowestOneBit(i) == i). 

The javadoc could probably be made clearer - but the only way to set 
alignment is "withAlignmentBits", and the javadoc there expects bit 
alignment to be a power of two and >= 8. So I think you can conclude 
that byte alignment must also be a power of two.

One thing which probably fell through the cracks, is that the alignment 
check does not apply when a layout is created - e.g. 
MemoryLayout.valueLayout(10, ByteOrder.nativeOrder()) succeeds, and sets 
alignment of 10, which is incorrect. Same is true for the recursive 
cases, e.g. when a struct/union/sequence layouts are created. I think 
some extra checks are required here. Also, if we want to allow sub-byte 
sizes (e.g. for describing bitmasks) we need to be able to accept bit 
alignment smaller (but still power of two) than 8, I think.

Maurizio