Comments on SegmentAllocator and ResourceScope
maurizio.cimadamore at oracle.com
Mon May 17 10:26:06 UTC 2021
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,
> 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.
More information about the panama-dev