On Tue, 1 Oct 2024 05:52:23 GMT, Eirik Bjørsnøs <eir...@openjdk.org> wrote:
>> As this stands (modulo my other comments) this change is mostly OK. Using >> the SOFT_MAX value within java.base is fine. Using SOFT_MAX within >> java.base-related tests is a little suspicious, because it requires the >> addition of directives that export an internal package from java.base ... >> but these tests are generally expected to be closely coupled with java.base >> internals, so they're probably ok too. >> >> I agree with the decision not to expand usage of SOFT_MAX further, as it's >> really just a guess at a VM implementation limit that's not defined >> anywhere. (There have been requests to make this public; see >> [JDK-8246725](https://bugs.openjdk.org/browse/JDK-8246725), which I'm >> opposed to.) The *real* fix here is to fix the JVM so that it can allocate >> arrays of Integer.MAX_VALUE. From time to time I hear rumors that this might >> happen at some point. Meanwhile, let's not expose something with ill-defined >> semantics like SOFT_MAX beyond java.base. >> >> As an aside, for your own edification, you might want to verify that >> compile-time constants are indeed inlined, by inspecting the >> before-and-after bytecode. This is well documented; see JLS 13.1, item 3 of >> the first numbered list in that section. I'm sure this is well tested, so >> there's no need for a test for this. > >> As this stands (modulo my other comments) this change is mostly OK. Using >> the SOFT_MAX value within java.base is fine. Using SOFT_MAX within >> java.base-related tests is a little suspicious, because it requires the >> addition of directives that export an internal package from java.base ... >> but these tests are generally expected to be closely coupled with java.base >> internals, so they're probably ok too. > > I was pondering a bit whether to include tests. The use of the constant > perhaps aids understanding even more here, because the value might seem "more > magic" , being further removed from where actual allocation takes place. > Tests are a useful tool for understanding APIs and their implementation, so > less magic feels like an improvement. Also, we want to update tests as well > if the implementation limit is lifted, this makes that update easier. > >> I agree with the decision not to expand usage of SOFT_MAX further, as it's >> really just a guess at a VM implementation limit that's not defined >> anywhere. (There have been requests to make this public; see >> [JDK-8246725](https://bugs.openjdk.org/browse/JDK-8246725), which I'm >> opposed to.) The _real_ fix here is to fix the JVM so that it can allocate >> arrays of Integer.MAX_VALUE. From time to time I hear rumors that this might >> happen at some point. Meanwhile, let's not expose something with ill-defined >> semantics like SOFT_MAX beyond java.base. > > Good, I initially included code outside `java.base`, but that quicly got > messy. > >> As an aside, for your own edification, you might want to verify that >> compile-time constants are indeed inlined, by inspecting the >> before-and-after bytecode. This is well documented; see JLS 13.1, item 3 of >> the first numbered list in that section. I'm sure this is well tested, so >> there's no need for a test for this. > > Thanks for the JLS pointer, good that this is not just a javac implementation > detail. > > Confirmed this using: > > % javap -c -p -constants java.io.InputStream | grep MAX_BUFFER_SIZE` > private static final int MAX_BUFFER_SIZE = 2147483639; > % javap -c -p -constants java.io.InputStream | grep ArraySupport @eirbjo Thanks for your thoughts on using the constant in the tests. It all makes good sense. @jaikiran Thanks for handling testing for this. ------------- PR Comment: https://git.openjdk.org/jdk/pull/21268#issuecomment-2386362465
