RFR: 8321033: Avoid casting Array to GrowableArray [v6]

[Kim Barrett]

On Fri, 14 Jun 2024 19:55:39 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> `ModuleEntry::_reads` is declared as an GrowableArray<ModuleEntry*>*, but when stored in a CDS archive, it's assigned to an Array<ModuleEntry*>*. To ensure better type safety, `ModuleEntry::_reads` is changed to a generic pointer which uses two different getters and setters as well as two booleans to ensure the value is interpreted correctly. This was chosen to avoid introducing a new field to the ModuleEntry class, as another pointer and further alignment would increase the size of the ModuleEntry array. Verified with tier1-5 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Moved state bool into initializer list
>  - Reverted assert

Changes requested by kbarrett (Reviewer).

src/hotspot/share/classfile/moduleEntry.cpp line 167:

> 165:     set_can_read_all_unnamed();
> 166:   } else {
> 167:     if (_reads == nullptr) {

I think this should be `reads()` rather than `_reads`, to verify we're in the expected state.

src/hotspot/share/classfile/moduleEntry.cpp line 252:

> 250:     int reads_len = reads()->length();
> 251:     for (int i = 0; i < reads_len; ++i) {
> 252:       f->do_module(reads()->at(i));

I think simpler and clearer would be

for (ModuleEntry* m : reads()) {
  f->do_module(m);
}

src/hotspot/share/classfile/moduleEntry.cpp line 277:

> 275:     _is_open(is_open),
> 276:     _is_patched(false),
> 277:     DEBUG_ONLY(_reads_is_archived(false) COMMA) {

This should be

_is_patched(false)
DEBUG_ONLY(COMMA _reads_is_archived(false)) {

Does the trailing COMMA version build?  (Either release or debug?)  I think it shouldn't, because
in either mode there is a extraneous trailing comma that I think the compiler should reject.

src/hotspot/share/classfile/moduleEntry.hpp line 124:

> 122:   bool             has_reads_list() const;
> 123:   GrowableArray<ModuleEntry*>* reads() const {
> 124:     DEBUG_ONLY(assert(!_reads_is_archived, "sanity"));

An assert is already debug-only, and doesn't need to be wrapped in a DEBUG_ONLY form.
Similarly below in archived_reads.

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

PR Review: https://git.openjdk.org/jdk/pull/19549#pullrequestreview-2119655202
PR Review Comment: https://git.openjdk.org/jdk/pull/19549#discussion_r1640583405
PR Review Comment: https://git.openjdk.org/jdk/pull/19549#discussion_r1640593373
PR Review Comment: https://git.openjdk.org/jdk/pull/19549#discussion_r1640594915
PR Review Comment: https://git.openjdk.org/jdk/pull/19549#discussion_r1640580327