RFR: 8333658: NMT: Use an allocator with 4-byte pointers to save memory in NativeCallStackStorage [v11]

[Johan Sjölen]

On Mon, 10 Jun 2024 14:12:06 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Now we use access specifiers to deny access to the index, except for the allocator, which is a friend. We do want the index to be opaque, so we do this. On top of that, we use an `_owner` field in debug mode to assert that we don't free to the wrong allocator.
>
>> Now we use access specifiers to deny access to the index, except for the allocator, which is a friend. We do want the index to be opaque, so we do this. On top of that, we use an `_owner` field in debug mode to assert that we don't free to the wrong allocator.
> 
> The last part I don't like. It causes `I` to quadruple in size in debug builds only. For something that is potentially used as member in bunch of different data structures, it can cause considerable deltas in memory layout between debug and release builds. I don't think that is a good thing. What you test in debug should be close to what's running at a customer.
> 
> I also don't think it is particularly useful. I would instead do a simple range check on I. That gives you a part of the sanity checks without the negatives. Couple that with a sanity check wrt to slot state (don't free free slots) you are pretty well covered already.
> 
> Other than that, I think using an own class to hide a simple i32 is unnecessary complex, but I leave that up to you. If you do this, please take care of constness. And make it work for its money, e.g. by giving it an is_nil instead of exposing nil, and requiring the user to manually compare it.

Alright, I get the counterpoint re: memory usage of the slots. I can live with that, though I am a bit disappointed as I had double-free coverage in mind also :).

>Other than that, I think using an own class to hide a simple i32 is unnecessary complex, but I leave that up to you. If you do this, please take care of constness. And make it work for its money, e.g. by giving it an is_nil instead of exposing nil, and requiring the user to manually compare it.

Yes, I'll make `_idx` const. `nil` is also exposed so that you can create nil pointers, which is important. I wanted to make the `I(int32_t idx)` constructor private, as only the allocator should construct pointers. This turned out to not be possible due to the design of `GrowableArray`.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18979#discussion_r1633841703