[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8319115: GrowableArray: Do not initialize up to capacity
From:       Emanuel Peter <epeter () openjdk ! org>
Date:       2023-12-20 12:58:53
Message-ID: Y_kYbKM0tLcOpgXQQX2FQAgdCEgAmbhGxSpdduTV3c4=.010f2ced-e90f-4646-bfb3-eec1285798f2 () github ! com
[Download RAW message or body]

On Mon, 18 Dec 2023 22:48:18 GMT, Kim Barrett <kbarrett@openjdk.org> wrote:

> > @eme64  Is it feasible to split this up to solve each of the problems you \
> > identify in stages? There is also overlap here with JDK-8319709 IIUC. Thanks.
> 
> > @dholmes-ora These are the "parts":
> > 
> > 1. initialize up to capacity vs length
> > 
> > 2. update the test to verify this (complete refactoring)
> > 
> > 3. remove cheap use of GrowableArray -> use GrowableArrayCHeap instead
> > 
> > 
> > The first 2 items are inseparable, I cannot make substantial changes to many \
> > GrowableArray methods without there even being tests for them. And the tests \
> > would not pass before the changes for item 1, since the tests also verify what \
> > elements of the array are initialized. So adding the tests first would not be \
> > very feasible. 
> > The 3rd item could maybe be split, and be done before the rest. Though it would \
> > also require lots of changes to the test, which then I would have to completely \
> > refactor with items 1+2 anyway. 
> > And the items are related conceptually, that is why I would felt ok pushing them \
> > together. It is all about when (item 1) and what kinds of (item 3) constructors / \
> > destructors are called for the elements of the arrays, and verifying that \
> > thoroughly (item 2). 
> > Hence: feasible probably, but lots of work overhead. Do you think it is worth it?
> 
> I too would prefer that it be split up.  It's very easy to miss important details \
> in amongst all the mostly relatively simple renamings.  That is, I think 3 should \
> be separate from the other changes.

@kimbarrett @dholmes-ora I just published this: \
https://github.com/openjdk/jdk/pull/17160 It removes the C-Heap capability from \
                `GrowableArray`, and replaces usages with `GrowableArrayCHeap`.
Bonus: we can now check that all element types of `GrowableArray` should be trivially \
destructible (that way it is always ok to abandon elements on the array, when the \
arena or ResourceMark go out of scope).

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

PR Comment: https://git.openjdk.org/jdk/pull/16918#issuecomment-1864429000


[prev in list] [next in list] [prev in thread] [next in thread]