8317279: Standard library implementation of value classes and objects#31123
8317279: Standard library implementation of value classes and objects#31123MrSimms wants to merge 2859 commits into
Conversation
Reviewed-by: liach, lmesnik
Reviewed-by: phubner, matsaave
Reviewed-by: chagedorn, dfenacci
Reviewed-by: fparain, qamai
…ing to bad casts with is_inlinetype() Reviewed-by: thartmann, qamai, dlong
Reviewed-by: qamai, thartmann
Co-authored-by: Axel Boldt-Christmas <aboldtch@openjdk.org> Co-authored-by: Joel Sikström <jsikstro@openjdk.org> Co-authored-by: Stefan Karlsson <stefank@openjdk.org> Reviewed-by: aboldtch, stefank, thartmann, fparain
…: must constrain OSR typestate Reviewed-by: thartmann
…es.java fails post jdk-27+16 Reviewed-by: vromero
…t001/TestDescription.java SIGSEGV in JvmtiTagMapKey::heapwalk_object Reviewed-by: sspitsyn
…ithXComp.java#xcomp-disable-tiered-compilation on linux-aarch64 Reviewed-by: dholmes
Reviewed-by: qamai
Reviewed-by: iwalulya, stefank
…ost barrier Reviewed-by: qamai, thartmann, stefank
…rs for strict fields Reviewed-by: liach, qamai, thartmann
…llable inline types in the calling convention Reviewed-by: qamai, chagedorn
…fastdebug Co-authored-by: Anton Seoane Ampudia <aseoane@openjdk.org> Reviewed-by: shade, fparain
…t with --enable-preview Reviewed-by: dholmes, sspitsyn
…y_preload_from_loadable_descriptors Reviewed-by: matsaave, aboldtch, fparain
… for preview features Reviewed-by: liach
Reviewed-by: chagedorn
…th COH Reviewed-by: thartmann
Reviewed-by: qamai, thartmann
…b class Co-authored-by: Stefan Karlsson <stefank@openjdk.org> Reviewed-by: phubner, fparain
Reviewed-by: lfoltan
Reviewed-by: alanb, sgehwolf, thartmann
Reviewed-by: mchevalier, thartmann
| } | ||
|
|
||
| /** | ||
| * Returns a ModuleFinder that locates modules in a JDK exploded image. | ||
| * @param modulesDir the modules directory ($JAVA_HOME/modules) |
There was a problem hiding this comment.
Is this is a typo? Should it have been $JAVA_HOME/jmods?
There was a problem hiding this comment.
This factory method is for the JDK exploded images, so $JAVA_HOME/modules is correct.
(The $JAVA_HOME/jmods directory is used for packaged modules, typically jlink is run with --keep-packaged-modules to save the packaged modules in that location).
There was a problem hiding this comment.
I hadn't paid attention that for exploded images the modules directory is named "modules". I just got confused when I saw that name, because in my mind modules meant the runtime image (regular file) that resides in $JAVA_HOME/lib directory.
Thank you for clarifying.
…flat arrays with oops Reviewed-by: iklam, fparain
| return reflectionFactory.parseAccessFlags(getModifiers(), | ||
| AccessFlag.Location.METHOD, | ||
| getDeclaringClass()); | ||
| return AccessFlagSet.ofValidated(AccessFlagSet.METHOD_FLAGS, getModifiers()); |
There was a problem hiding this comment.
Before this change, the declaring class' class file version was being taken into account to decide the returned AccessFlags. That no longer seems to be the case now and this implementation uses the latest class file version in this decision making. Is that intentional? Would a call to
java.lang.reflect.AccessFlag#maskToAccessFlags(int mask, Location location, ClassFileFormatVersion cffv)
be more appropriate here?
There was a problem hiding this comment.
Yes. If you look at hotspot's representation of access_flags, you will realize there is one uniform representation - older flags from previous formats gets translated to that uniform representation in ClassFileParser. And that uniform representation is NOT the latest class file version's maskToAccessFlags: for example, it currently permits useless ACC_STRICT bit for methods to be returned by getModifiers() and accessFlags(), not allowed by the JVMS. This is similar to how later class file versions don't allow jsr/ret but hotspot supports them anyways.
There was a problem hiding this comment.
So for each location and preview on/off, there is one uniform way to parse the hotspot representation. Many locations have the same configuration or compatible preview on/off configurations for the same location; in case of compatible configurations, I use the more general flags set, like I use PreviewAccessFlags.FIELD_PREVIEW_FLAGS for Fields, because making another switch to AccessFlagSet.FIELD_FLAGS for preview off accomplishes the same behavior. The only place where the preview on configuraiton is incompatible with the preview off configuration is Class and inner class access flags, which is handled in Class::accessFlags.
Reviewed-by: alanb, vromero
….java#StressIncrementalInliningDontInlineMyAbstractInit timed out Reviewed-by: phubner, chagedorn
…e cleared yet Reviewed-by: qamai, vlivanov
Reviewed-by: jvernee, vromero
Reviewed-by: vromero
…zation methods on value objects Reviewed-by: dholmes, alanb
|
@MrSimms |
|
/labels remove compiler,hotspot,shenandoah |
|
@liach Unknown command |
|
/label remove compiler,hotspot,shenandoah |
openjdk
Bot
removed
compiler
|
@liach The The |
|
|
||
| /// {@return whether this field type may store value objects} | ||
| /// This excludes primitives and includes Object. | ||
| public static boolean isValueObjectCompatible(Class<?> fieldType) { |
There was a problem hiding this comment.
Nit - some methods on this class use /// and some others use /** style javadoc. Can it be made consistent?
|
|
||
| @ParameterizedTest | ||
| @MethodSource("valueObjects") | ||
| public void roundTrip(Object expected) throws Exception { |
There was a problem hiding this comment.
I think this test class and this test method in particular with need some attention. The summary of this test says "Serialize and deserialize value objects". This roundTrip() method sources its param from "valueObjects" method. The valueObjects() method a few lines above constructs instances of SimpleValue class. SimpleValue class (some lines below) is declared as:
public static class SimpleValue implements Serializable
It's an identity class. So, unless I'm missing something, this test method isn't testing anything related to value objects. I initially thought this might be an oversight in the declaration of the SimpleValue class and it probably was missing the value modifier. But this test method expects instances of SimpleValue to be serialized and deserialized without any exceptions, which is contrary to what is being proposed for value objects in context of JEP-401.
There was a problem hiding this comment.
I've filed https://bugs.openjdk.org/browse/JDK-8387398 to track this.
| @ParameterizedTest | ||
| @MethodSource("migrationObjects") | ||
| public void treeVTest(Object origObj, String origName, String replName, Object expectedObject) throws Exception { | ||
| byte[] bytes = serialize(origObj); |
There was a problem hiding this comment.
This test method too I think will need some clean up. The expectedObject passed to this method is never an instance of Exception so the try/catch block in this test method shouldn't exist and the:
@param expectedObject the expected object (graph) or an exception if it should fail
should be updated to remove the mention of the exception.
| new SerializablePoint(2, 6)}), | ||
| Arguments.of(Arguments.of( | ||
| new SerializablePoint(3, 7), | ||
| new SerializablePoint(4, 8))), |
There was a problem hiding this comment.
This test used to be testng test which was later converted to junit. It looks like that conversion might have introduced a bug in this parameter generation. Previously, when this test was testng it used to be:
@DataProvider(name = "ImplementSerializable")
public Object[][] implementSerializable() {
return new Object[][]{
new Object[]{new SerializablePoint(11, 101)},
new Object[]{new SerializablePoint[]{
new SerializablePoint(1, 5),
new SerializablePoint(2, 6)}},
new Object[]{new Object[]{
new SerializablePoint(3, 7),
new SerializablePoint(4, 8)}},
new Object[]{new SerializableFoo(45)},
new Object[]{new SerializableFoo[]{new SerializableFoo(46)}},
new Object[]{new ExternalizableFoo("hello")},
new Object[]{new ExternalizableFoo[]{new ExternalizableFoo("there")}},
};
}
The conversion has led to:
Arguments.of(Arguments.of(
new SerializablePoint(3, 7),
new SerializablePoint(4, 8))),
which is wrong. As a result, right now, instead of running the implementSerializable(Object obj) test method 7 times with different paramters, that test method only gets run twice:
[01:38:50.773] STARTED ValueSerializationTest::implementSerializable '[1] [SerializablePoint x=11 y=101]'
[01:38:50.799] SUCCESSFUL ValueSerializationTest::implementSerializable '[1] [SerializablePoint x=11 y=101]' [25ms]
[01:38:50.802] STARTED ValueSerializationTest::implementSerializable '[2] [[SerializablePoint x=1 y=5], [SerializablePoint x=2 y=6]]'
[01:38:50.808] SUCCESSFUL ValueSerializationTest::implementSerializable '[2] [[SerializablePoint x=1 y=5], [SerializablePoint x=2 y=6]]' [6ms]
The rest parameters seems to be ignored due to the Arguments.of(Arguments.of(...)) bug. A similar issue exists in the parameters provider for another test method in this class. I'll file an issue and address this shortly.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hello Christian @sormuras, would you happen to know if there are ways to catch these issues with parameterized tests by failing the test?
| * <li>The class does not have an accessible no-arg constructor | ||
| * <li>The ObjectStreamClass of an enum constant does not represent | ||
| * an enum type | ||
| * <li>A {@linkplain Class#isValue value class} cannot be serialized |
There was a problem hiding this comment.
I think this might have to be a bit more precise. In its current form this seems to imply that an instance of a value class cannot be serialized, which isn't accurate because the primitive wrapper classes (for example java.lang.Integer) which are value classes when preview is enabled can be serialized/deserialized. So can instances of value classes like java.time.Instant which implement the writeReplace() method to write out a replacement object to facilitate serialization.
There was a problem hiding this comment.
If it gets too wordy, then maybe we can just drop the mention of value classes, since there's already a "Other conditions given in the Java Object Serialization Specification" on the next line?
There was a problem hiding this comment.
I think saying something like "the class is a value class" works - the no-arg scenario doesn't mean no classes without no-arg constructor could be serialized.
| @@ -32,6 +32,8 @@ <h1 id="ValueBased">{@index "Value-based Classes"}</h1> | |||
|
|
|||
| Some classes, such as {@code java.lang.Integer} and | |||
| {@code java.time.LocalDate}, are <em>value-based</em>. | |||
| The compiler and runtime enforce the value based properties below if it is declared | |||
| as {@code value class} and preview features are enabled. | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
the class does not declare (or discourages use of) accessible constructors;
Does this apply for value classes that are being proposed in this JEP? As far as I remember, I haven't seen any text which states or discourages the use of constructors for value object construction.
In fact, on the contrary, in one of the review comments in this PR for a different discussion, it has been rightly pointed out that in recent times there have been efforts to undeprecate previously deprecated-for-removal constructors of the primitive wrapper classes. For example https://bugs.openjdk.org/browse/JDK-8354335.
| @@ -49,6 +49,7 @@ | |||
| * remove entries automatically when the key is garbage collected. This is | |||
| * accomplished by using a backing map where the keys are either a | |||
| * {@link WeakReference} or a {@link SoftReference}. | |||
| * Keys must be {@linkplain Class#isIdentity() identity objects.} | |||
There was a problem hiding this comment.
There is a typo here. Class doesn't have an isIdentity() method in this version of the JEP implementation. I'll file an issue to fix it and to link it to Class.isValue().
There was a problem hiding this comment.
Hmm. There are build warning for javadoc link issues like that. If those warnings were disabled, they should be re-enabled before integration.
There was a problem hiding this comment.
Hello Joe, this jdk.internal.util.ReferencedKeyMap is an internal class of java.base module, exported only to select modules. So I suspect these checks aren't run against such classes during the build (I maybe wrong, I haven't checked).
I've filed https://bugs.openjdk.org/browse/JDK-8387495 with a PR available for review openjdk/valhalla#2603
There was a problem hiding this comment.
Hello Joe, this
jdk.internal.util.ReferencedKeyMapis an internal class ofjava.basemodule, exported only to select modules. So I suspect these checks aren't run against such classes during the build (I maybe wrong, I haven't checked).I've filed https://bugs.openjdk.org/browse/JDK-8387495 with a PR available for review openjdk/valhalla#2603
Oh, I overlooked that detail; thanks.
I looking at the makefile changes and all the expected checks for the public types looked to be in place.
|
These changes for JEP 401 add preview APIs in a new way compared to prior JEPs. In particular, there are different versions of platform classes with different behavioral semantics with and without preview being enabled. Given that, I think it would be helpful to document the mechanisms used to implement this novel arrangement, say by a new file in the I'd like to see this file describe:
The classes with different version with and without preview enabled could then contain a source-only comment pointing to this file so maintains of those files would know where to go to get more context. |
20 participants
This is a "sub-review pull request" for the first preview of JEP 401: Value Classes and Objects, specifically JDK-8317279: Standard library implementation of value classes and objects
Note
This pull request and the other sub-review pull requests listed below are based on the "master pull request" (#31120). It contains the same full set of code changes as the "master pull request" to preserve the full implementation context; the language compiler, JVM, and standard library changes are intertwined. This separate pull requests exist only to subdivide the review and related discussion by area.
Other areas for review:
Code changes resulting from the review process should be made in
valhalla/lworld.valhalla/lworldis currently updated fromjdk/masterwhenever a weeklyjdktag is created. At that time, code changes fromvalhalla/lworldwill be propagated to the master pull request and to all sub-review pull requests, including this one.Ultimately, review sign-off will be recorded on the "master pull request", and this pull request will be closed without integration.
This pull request has a large code surface area and often conflicts with
jdk/masteron a daily basis. Refer tovalhalla/lworldfor the latest state of the project code, keeping in mind that it may lag several days behindjdk/master. Both repositories may be needed as references during review.PR implementation description
Here's a high-level overview of what's included here.
Core object behaviors
Introduced
Objectsmethods to test for identity andIdentityExceptionfortest failures; revised definitions of
==andidentityHashCodeto work on thefields of value objects.
src/java.base/share/classes/java/langsrc/java.base/share/classes/java/utilsrc/java.base/share/classes/java/lang/runtimetest/jdk/java/utiltest/jdk/valhalla/valuetypestest/micro/org/openjdk/bench/valhallaValue class migration
Generated and compiled a value class version of certain core JDK classes
(primitive wrappers,
Optional*,java.time,Number,Record) when previewfeatures are enabled; mentioned value classes in the documentation for
"value-based class".
make/modules/java.basesrc/java.base/share/classes/java/langsrc/java.base/share/classes/java/utilsrc/java.base/share/classes/java/timesrc/java.base/share/classes/java/lang/doc-filessrc/java.base/share/classes/jdk/internal/MigratedValueClass.javatest/jdk/java/utiltest/jdk/valhalla/valuetypes/IdentityReflectionTest.javaPreview class deployment
Added tooling support for preview class files stored in a
META-INF/previewfolder, to be used when preview features are enabled.
makesrc/java.base/share/classes/jdk/internal/jtrfssrc/java.base/share/classes/jdk/internal/modulesrc/jdk.jdeps/share/classes/com/sun/tools/jdeprscantest/jdk/java/lang/moduletest/jdk/jdk/internal/jimagetest/jdk/jdk/internal/jtrfstest/jdk/tools/jlinktest/micro/org/openjdk/bench/jdk/internal/jrtfsNew field layouts & behaviors
MethodHandlesandVarHandlesupdated to support new kinds of field andarray layouts, and to support strictly-initialized static field validation. This
includes new lambda forms, etc., to interact with flattened field layouts, which
may or may not be atomic and may or may not permit nulls. (Some layouts are only
possible using internal-only APIs. All layouts share the same descriptor and
reflective
Class.)make/modules/java.basesrc/java.base/share/classes/java/lang/invokesrc/java.base/share/classes/java/util/Arrays.javasrc/java.base/share/classes/jdk/internal/miscsrc/java.base/share/classes/jdk/internal/reflectsrc/java.base/share/classes/jdk/internal/value/ValueClass.javasrc/java.base/share/classes/jdk/internal/vmsrc/jdk.unsupportedtest/jdk/java/lang/invoketest/jdk/valhalla/valuetypestest/micro/org/openjdk/bench/valhallaCore reflection
Added support for the
ACC_IDENTITY(sometimes thevaluelanguage modifier)and
ACC_STRICT_INITaccess flags; some changes toAccessFlagto align withVM handling of class file versions.
src/java.base/share/classes/java/lang/Class.javasrc/java.base/share/classes/java/lang/reflectsrc/java.base/share/classes/jdk/internal/reflectsrc/java.base/share/classes/jdk/internal/accesstest/jdk/java/lang/Classtest/jdk/java/lang/reflectSerialization
Adjusted handling of value classes (and other classes with
strictly-initialized fields) so that, aside from records, they throw an
exception on attempts to serialize/deserialize without
writeReplace/readResolve. Special, private annotation-based mechanismimplemented for the primitive wrapper classes.
src/java.base/share/classes/java/iosrc/java.base/share/classes/java/lang/reflect/ReflectionFactory.javasrc/java.base/share/classes/jdk/internal/value/DeserializeConstructor.javatest/jdk/java/io/SerializableReference API
Disallowed value object use with the Reference API and
WeakHashMap.src/java.base/share/classes/java/lang/refsrc/java.base/share/classes/java/util/WeakHashMap.javasrc/java.base/share/classes/jdk/internal/utiltest/jdk/java/lang/reftest/jdk/java/util/WeakHashMapValues.javaClassFile API
Added support for new
ACC_IDENTITYandACC_STRICT_INITmodifiers; supportfor generating
early_larval_frames inStackMapTable; and basic support forthe
LoadableDescriptorsattribute.src/java.base/share/classes/java/lang/classfilesrc/java.base/share/classes/jdk/internal/classfiletest/jdk/jdk/classfilejavap support
Updated to support new class file flags and attributes.
src/jdk.jdepstest/langtools/tools/javapTesting support
Processors for
@AsValueClassand@StrictInittesting annotations; improvedsupport for testing with preview features turned on and off.
test/libtest/lib-testtest/jtreg_value_class_plugindocLanguage implementation
These can be ignored, they are covered by JDK-8317277:
src/java.compilersrc/jdk.compilertest/langtools/jdk/javadoctest/langtools/tools/javactest/langtools/tools/libJVM implementation
These can be ignored, they are covered by JDK-8317278:
.github/workflowssrc/hotspotsrc/java.base/share/nativesrc/java.se/share/data/jdwpsrc/jdk.hotspot.agentsrc/jdk.jdisrc/jdk.jdwp.agentsrc/jdk.jfrsrc/utils/IdealGraphVisualizersrc/utils/LogCompilationtest/hotspottest/jdk/com/sun/jditest/jdk/java/lang/instrumenttest/jdk/jdk/internal/vmtest/jdk/jdk/jfrAPI documentation can be found by clicking the "API" link at https://cr.openjdk.org/~dlsmith/jep401/latest
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/31123/head:pull/31123$ git checkout pull/31123Update a local copy of the PR:
$ git checkout pull/31123$ git pull https://git.openjdk.org/jdk.git pull/31123/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 31123View PR using the GUI difftool:
$ git pr show -t 31123Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/31123.diff
Using Webrev
Link to Webrev Comment