On Wed, 11 Jan 2023 15:08:23 GMT, Stefan Karlsson <[email protected]> wrote:
>>> I'm happy to see this flag getting removed. I'm less happy about seeing
>>> usages of the array allocators being replaced by macros. I'd rather see an
>>> effort towards getting rid of these macros. Could we limit this patch to
>>> only remove the ArrayAllocatorMallocLimit flag and ArrayAllocator class,
>>> and defer the discussion around what API to use for array allocations?
>>
>> `ArrayAllocator` with `ArrayAllocatorMallocLimit` removed is effectively
>> `MallocArrayAllocator`. Are you suggesting leaving `MallocArrayAllocator`
>> and `MmapArrayAllocator` thus update references to `ArrayAllocator` to be
>> `MallocArrayAllocator`?
>>
>> As far as APIs, the majority of the codebase uses the macros. IMO
>> consistency would be better and having two ways of doing things doesn't
>> help. But if you feel strongly about it, we can punt and just surgically
>> remove the bare minimum, assuming you clarify your expectation (see above).
>
>> ArrayAllocator with ArrayAllocatorMallocLimit removed is effectively
>> MallocArrayAllocator. Are you suggesting leaving MallocArrayAllocator and
>> MmapArrayAllocator thus update references to ArrayAllocator to be
>> MallocArrayAllocator?
>
> Yes, that was what I wanted.
>
>> As far as APIs, the majority of the codebase uses the macros. IMO
>> consistency would be better and having two ways of doing things doesn't
>> help. But if you feel strongly about it, we can punt and just surgically
>> remove the bare minimum, assuming you clarify your expectation (see above).
>
> I agree about the argument about consistency, so I retract my objection. We
> can deal with these macros as a separate RFE (if we ever get to it).
>
> I would like to retain the usage of mmapped memory for ZGC to minimize the
> risk of any surprises for us. ZGC code also tend to initialize as much as
> possible in the initialization list, so the extra memset that comes after
> initialization lists sticks out a bit. Could you create a private
> `ZGranuleMap::allocate_array` function that uses
> os::reserve_memory/commmit_memory and change the code to be:
>
> inline ZGranuleMap<T>::ZGranuleMap(size_t max_offset) :
> _size(max_offset >> ZGranuleSizeShift),
> _map(allocate_array(_size)) {
>
>
> Or if you like, I can provide a patch on top of your branch to do that.
Applied your patch @stefank
-------------
PR: https://git.openjdk.org/jdk/pull/11931
