On Wed, 30 Apr 2025 15:33:52 GMT, Per Minborg <pminb...@openjdk.org> wrote:
>> This PR is based on the work of @mernst-github and aims to implement an
>> _internal_ thread-local 'stack' allocator, which works like a dynamically
>> sized arena, but with reset functionality to reset the allocated size back
>> to a certain level. The underlying memory could stay around between calls,
>> which could improve performance.
>>
>> Re-allocated segments are not zeroed between allocations.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve on comments
test/jdk/java/foreign/TestBufferStack.java line 192:
> 190: assertEquals(firstAddress, segment.address());
> 191: var segmentTwo = arena.allocate(JAVA_INT);
> 192: assertNotEquals(firstAddress, segmentTwo.address());
Should this check be stronger -- e.g. that the address of the second segment
should be the address of the first + the size of JAVA_INT?
test/jdk/java/foreign/TestBufferStack.java line 203:
> 201:
> 202: @Test
> 203: void allocationCaptureStateLayout() {
This test is not really testing much re. capture state layout. It feels like
the only added thing w.r.t. `allocationSameAsPoolSize` is that it also checks
that additional allocation will fail with IOOBE -- maybe these two tests should
be merged?
test/jdk/java/foreign/TestBufferStack.java line 241:
> 239: @Test
> 240: void closeConfinement() {
> 241: var pool = BufferStack.of(POOL_SIZE);
Nice confinement tests!
test/jdk/java/foreign/TestBufferStackStress.java line 28:
> 26: * @modules java.base/jdk.internal.foreign
> 27: * @build NativeTestHelper TestBufferStackStress
> 28: * @run junit/othervm --enable-native-access=ALL-UNNAMED
> TestBufferStackStress
is `--enable-native-access` needed here (and also in the other stress test) ?
test/jdk/java/foreign/TestBufferStackStress2.java line 45:
> 43: import java.util.concurrent.atomic.AtomicBoolean;
> 44:
> 45: final class TestBufferStackStress2 extends NativeTestHelper {
This also extends `NativeTestHelper` -- does it need to?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24829#discussion_r2070124607
PR Review Comment: https://git.openjdk.org/jdk/pull/24829#discussion_r2070126160
PR Review Comment: https://git.openjdk.org/jdk/pull/24829#discussion_r2070126843
PR Review Comment: https://git.openjdk.org/jdk/pull/24829#discussion_r2070129639
PR Review Comment: https://git.openjdk.org/jdk/pull/24829#discussion_r2070129263
