On Tue, 1 Oct 2024 05:39:09 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> Please review this cleanup PR which updates code and tests in `java.base` to >> consistently use `jdk.internal.util.ArraySupport.SOFT_MAX_ARRAY_LENGTH` >> when referring to the JVM's maximum array size implementation limit. >> Currently, instances of `Integer.MAX_VALUE - 8` are found across the code >> base, with varying degrees of documentation. It would be good to consolidate >> on a single source of truth, with proper documentation. >> >> This PR is a follow-up to #20905 where the same change was requested in >> `java.util.zip`. >> >> My understanding is that javac will fold this constant value into the byte >> code of the compiled use sites, as such this change should not affect class >> loading or cause startup issues. >> >> Instances selected for this PR were found searching for "Integer.MAX_VALUE - >> 8". The PR replaces these with `ArraySupport.SOFT_MAX_ARRAY_LENGTH`, while >> trimming or amending some code comments where appropriate. >> (`SOFT_MAX_ARRAY_LENGTH` already has a good explainer which does not need >> repetition at each use site). >> >> I also searched for instances of `Integer.MAX_VALUE - 1` and >> `Integer.MAX_VALUE - 2`, no convincing candidates were found. >> >> Instances outside `java.base` were deliberately left out to limit the scope >> and review cost of this PR. >> >> Tests updated to use `SOFT_MAX_ARRAY_LENGTH` are updated with the jtreg tag >> `@modules java.base/jdk.internal.util`. >> >> Testing: No new tests are added in this PR, the `noreg-cleanup` label is >> added to the JBS. The five affected tests have been run manually. GHA tests >> run green. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove comment and variable from HugeCapacity tests Hello Eirik, these changes look OK to me. Referring to this constant instead of the `Integer.MAX_VALUE - 8` is useful for the context of that value, especially in some places like the usage in `Pattern` class. As for the test changes, I think it's OK to refer this there too, given that several of these tests are actually wanting to use a value which implies the maximum limit on the array length. >From what I remember, adding a `@modules` in test definition makes the test >run as "othervm" (even if othervm isn't explicitly specified). But I think >that shouldn't matter much, at least not in this PR, because all the test >definitions that are changed in this PR are already othervm (explicitly). I am going to run tier tests against this PR in our CI. Please wait for that run to complete, before integrating. ------------- Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/21268#pullrequestreview-2339549362
