8352750: Use jimage for preview enabled value classes #1409

david-beaumont · 2025-03-26T10:49:27Z

Draft pull request. Final description TBD.

Progress

Change must not contain extraneous whitespace

Integration blocker

⚠️ Title mismatch between PR and JBS for issue JDK-8352750

Issue

JDK-8352750: [lworld] Use jimage for preview enabled value classes (Enhancement - P3) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1409/head:pull/1409
$ git checkout pull/1409

Update a local copy of the PR:
$ git checkout pull/1409
$ git pull https://git.openjdk.org/valhalla.git pull/1409/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1409

View PR using the GUI difftool:
$ git pr show -t 1409

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1409.diff

david-beaumont · 2025-03-26T10:50:04Z

src/hotspot/share/classfile/classLoader.cpp

@@ -97,9 +97,18 @@ static JImageClose_t                   JImageClose            = nullptr;
 static JImageFindResource_t            JImageFindResource     = nullptr;
 static JImageGetResource_t             JImageGetResource      = nullptr;

-// JimageFile pointer, or null if exploded JDK build.


bridgekeeper · 2025-03-26T10:50:26Z

👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into lworld will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-26T10:50:54Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

david-beaumont · 2025-03-26T10:51:36Z

src/hotspot/share/classfile/classLoader.cpp

+  JIMAGE_MODE_ENABLE_PREVIEW = 2
+};
+static JImageMode                      JImage_mode            = JIMAGE_MODE_UNINITIALIZED;
+


Private static state. I don't like it being static, but everything else in ClassLoader is, so I figured put it next to the thing it affects.

david-beaumont · 2025-03-26T10:52:56Z

src/hotspot/share/classfile/classLoader.cpp

@@ -154,7 +163,7 @@ void ClassLoader::print_counters(outputStream *st) {

 GrowableArray<ModuleClassPathList*>* ClassLoader::_patch_mod_entries = nullptr;
 GrowableArray<ModuleClassPathList*>* ClassLoader::_exploded_entries = nullptr;
-ClassPathEntry* ClassLoader::_jrt_entry = nullptr;


Reflect the true type of _jrt_entry (this is useful later).

david-beaumont · 2025-03-26T10:55:26Z

src/hotspot/share/classfile/classLoader.cpp

 JImageFile* ClassPathImageEntry::jimage_non_null() const {
  assert(ClassLoader::has_jrt_entry(), "must be");
-  assert(jimage() != nullptr, "should have been opened by ClassLoader::lookup_vm_options "
+  assert(JImage_file != nullptr, "should have been opened by ClassLoader::lookup_vm_options "
                           "and remained throughout normal JVM lifetime");


By using the field directly in these 4 places where jimage() used to be called, we remove a public method from ClassPathEntry, which greatly improves encapsulation, which is important for ensuring JImage use is consistent with respect to --enable-preview.

david-beaumont · 2025-03-26T10:57:40Z

src/hotspot/share/classfile/classLoader.cpp

+
+bool ClassPathImageEntry::is_preview_enabled() const {
+  assert(JImage_mode != JIMAGE_MODE_UNINITIALIZED, "uninitialized JImage reference");
+  return JImage_mode == JIMAGE_MODE_ENABLE_PREVIEW;
 }


Helper function to encapsulate the static field read and ensure it's only used inside ClassPathImageEntry.
While other code could directly access JImage_mode, it's easier to audit when going through this function (and would be easier to refactor later if it stops being a static field). It also adds a guard to prevent use-before-init.

david-beaumont · 2025-03-26T10:59:04Z

src/hotspot/share/classfile/classLoader.cpp

@@ -389,6 +399,8 @@ ClassPathImageEntry::ClassPathImageEntry(JImageFile* jimage, const char* name) :
  ClassPathEntry() {
  guarantee(jimage != nullptr, "jimage file is null");
  guarantee(name != nullptr, "jimage file name is null");
+  guarantee(JImage_mode != JIMAGE_MODE_UNINITIALIZED, "jimage is uninitialized");
+


I made this a guarantee to match the null pointer checks above (this should be the only place, other than is_preview_enabled() where JImage_mode is read.

david-beaumont · 2025-03-26T11:01:19Z

src/hotspot/share/classfile/classLoader.cpp

@@ -407,16 +419,19 @@ ClassFileStream* ClassPathImageEntry::open_stream(JavaThread* current, const cha
 //     2. A package is in at most one module in the jimage file.
 //
 ClassFileStream* ClassPathImageEntry::open_stream_for_loader(JavaThread* current, const char* name, ClassLoaderData* loader_data) {
+  JImageFile* jimage = this->jimage_non_null();
+  bool is_preview = this->is_preview_enabled();
+


Read related fields together and only once. Now this is the only place where either of these fields are obtained (and guards are present to ensure it happens after final initialization of flags) we know it's the only bit of code that needs modifying later to handle is_preview. For now the flag is ignored however.

david-beaumont · 2025-03-26T11:02:12Z

src/hotspot/share/classfile/classLoader.cpp

  jlong size;
-  JImageLocationRef location = (*JImageFindResource)(jimage_non_null(), "", get_jimage_version_string(), name, &size);
+  JImageLocationRef location = (*JImageFindResource)(jimage, "", get_jimage_version_string(), name, &size);



Rather than call the function multiple times, it seemed clearer to read it (and the new flag) just once at the start of this function.

david-beaumont · 2025-03-26T11:05:50Z

src/hotspot/share/classfile/classLoader.cpp

          _jrt_entry = new ClassPathImageEntry(JImage_file, canonical_path);
          assert(_jrt_entry != nullptr && _jrt_entry->is_modules_image(), "No java runtime image present");
-          assert(_jrt_entry->jimage() != nullptr, "No java runtime image");
        } // else it's an exploded build.


This final assert is very very redundant given the asserts around it and in the ClassPathImageEntry constructor. I removed it because it is otherwise a call to the old jimage() method outside the ClassPathImageEntry namespace (which means I would need to leave a public method that returns the JImageFile*, which I think is quite bad. The actual call to jimage() would just be reading the static field anyway, which we already know isn't null from (original) line 676.

david-beaumont · 2025-03-26T11:08:15Z

src/hotspot/share/classfile/classLoader.cpp

+    JImage_mode = enable_preview ? JIMAGE_MODE_ENABLE_PREVIEW : JIMAGE_MODE_DEFAULT;
+  }
+}
+


Called from arguments.cpp after flags are parsed.

david-beaumont · 2025-03-26T11:09:21Z

src/hotspot/share/classfile/classLoader.hpp

@@ -107,19 +105,29 @@ class ClassPathZipEntry: public ClassPathEntry {
 };


-// For java image files
+// A singleton path entry which takes ownership of the initialized JImageFile
+// reference. Not used for exploded builds.
 class ClassPathImageEntry: public ClassPathEntry {


I added some comments while working since there is subtlety here and things weren't very clear to me. It's important, for example, to know that this is explicitly intended as a singleton instance.

david-beaumont · 2025-03-26T11:11:03Z

src/hotspot/share/classfile/classLoader.hpp

+  // Returns the non-null, initialized JImage file reference.
+  JImageFile* jimage_non_null() const;
+  // Returns whether the JImage file will return resources suitable for a preview JVM.
+  bool is_preview_enabled() const;
 public:


Having these functions private guarantees nobody else can read (and potentially) misuse the JImageFile* outside the ClassPathImageEntry class. This is very important for improving encapsulation, which was somewhat lacking before.

david-beaumont · 2025-03-26T11:13:00Z

src/hotspot/share/classfile/classLoader.hpp

 public:
  bool is_modules_image() const;
  const char* name() const { return _name == nullptr ? "" : _name; }
-  JImageFile* jimage() const;
-  JImageFile* jimage_non_null() const;
+  // Called to closes the JImage during os::abort (normally not called).
  void close_jimage();


Comment added here because it's otherwise confusing to see a "close" method in a C++ object, because you naturally expect a destructor to be used. This is optional, but I think it helps people understand that this isn't "normal" semantics.

david-beaumont · 2025-03-26T11:18:50Z

src/hotspot/share/classfile/classLoader.hpp

@@ -331,7 +339,7 @@ class ClassLoader: AllStatic {

  // Modular java runtime image is present vs. a build with exploded modules
  static bool has_jrt_entry() { return (_jrt_entry != nullptr); }
-  static ClassPathEntry* get_jrt_entry() { return _jrt_entry; }
+  static ClassPathImageEntry* get_jrt_entry() { return _jrt_entry; }
  static void close_jrt_image();


This is a slight break in encapsulation since it's called from outside classLoader.cpp (in filemap.cpp). I could change it back to ClassPathEntry* if desired, but leave the static field type as ClassPathImageEntry*. Nobody calling this from outside cares about the type.

RogerRiggs · 2025-03-26T18:48:50Z

src/hotspot/share/classfile/classLoader.hpp

@@ -63,8 +63,6 @@ class ClassPathEntry : public CHeapObj<mtClass> {
  // Is this entry created from the "Class-path" attribute from a JAR Manifest?
  virtual bool from_class_path_attr() const { return false; }
  virtual const char* name() const = 0;
-  virtual JImageFile* jimage() const { return nullptr; }
-  virtual void close_jimage() {}


I'm not sure but this provides close_jimage for ClassPathZipEntry; though it is a no-op, as usual for zip files.

Yes, but it seems nobody calls it virtually and everything compiles fine without it, so I'm calling that "dead code".
Much better encapsulation without this.

RogerRiggs · 2025-04-02T14:54:34Z

src/hotspot/share/cds/filemap.cpp

+//    if (err != nullptr) {
+//      log_warning(cds)("This archive was %s with --enable-preview -XX:+EnableValhalla. It is "
+//                         "incompatible with the current JVM setting", err);
+//      return false;
+//    }
  }


This code should be restored. It should operate correctly (with other fixes in place).

RogerRiggs · 2025-04-02T14:55:03Z

make/GenerateLinkOptData.gmk

@@ -66,7 +66,7 @@ endif
 #   default classlist is minimal, let's filter out the '@cp' lines until we can
 #   find a proper solution.
 CLASSLIST_FILE_VM_OPTS = \
-    -Duser.language=en -Duser.country=US
+    -Duser.language=en -Duser.country=US -Dvalue.bsm.debug=true


Can be removed.

RogerRiggs · 2025-04-02T14:55:15Z

make/GenerateLinkOptData.gmk

@@ -77,7 +77,7 @@ $(CLASSLIST_FILE): $(INTERIM_IMAGE_DIR)/bin/java$(EXECUTABLE_SUFFIX) $(CLASSLIST
 	$(FIXPATH) $(INTERIM_IMAGE_DIR)/bin/java -XX:[email protected] \
 	    $(CLASSLIST_FILE_VM_OPTS) \
 	    -cp $(SUPPORT_OUTPUTDIR)/classlist.jar \
-	    build.tools.classlist.HelloClasslist $(LOG_DEBUG)
+	    build.tools.classlist.HelloClasslist $(LOG_VERBOSE)


Can be removed.

* Reenable validation * Undoing debug stuff. * Fix daft preview flag mistake. * [[AUTOMATIC FORMATTING]] * WIP - push for Roger. * Builds to completion for "make images" but value tests don't run. * Fix build issue with copied files. * Automatic Java reformat * Work in progress - builds but no tests yet. * Tidying up inconsistencies. * More encapsulation of JImage_file and mode into private static functions. Now the preview flag is plumbed into the JImage code, but not acted upon yet. * Change method return back to base class type (nobody relies on the sub-type). * Isolate JImage use within ClassPathImageEntry.

liach · 2025-04-15T23:50:55Z

src/jdk.jlink/share/classes/jdk/tools/jmod/JmodTask.java

                        .map(path -> toPackageName(path))
-                        .filter(pkg -> pkg.length() > 0)
+                        .flatMap(Optional::stream)


Maybe mapMulti(Optional::ifPresent)?

That doesn't actually work, since it ends up with a sequence of Object, not String.

What if .<String>mapMulti(Optional::ifPresent)?

I mean maybe (I'm sure it can be made to work), but is that any clearer at this stage?

This is mainly for performance - creating new streams have some overhead, because streams are stateful with closing, flags, etc.

Oh interesting. I had always assumed that Optional's stream() method would return something very cheap, since it has exactly one or zero elements. I'll change it then (and add a note). Thanks for educating me!

david-beaumont commented Mar 26, 2025

View reviewed changes

RogerRiggs reviewed Mar 26, 2025

View reviewed changes

david-beaumont force-pushed the jdk_8352750 branch from 07ffe40 to 90888b3 Compare March 29, 2025 00:01

RogerRiggs reviewed Apr 2, 2025

View reviewed changes

david-beaumont force-pushed the jdk_8352750 branch from ad9d665 to e0d7959 Compare April 8, 2025 21:22

liach reviewed Apr 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8352750: Use jimage for preview enabled value classes #1409

8352750: Use jimage for preview enabled value classes #1409

david-beaumont commented Mar 26, 2025 •

edited by openjdk bot

Loading

david-beaumont Mar 26, 2025

bridgekeeper bot commented Mar 26, 2025

openjdk bot commented Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025 •

edited

Loading

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

david-beaumont Mar 26, 2025

RogerRiggs Mar 26, 2025

david-beaumont Mar 26, 2025

RogerRiggs Apr 2, 2025

RogerRiggs Apr 2, 2025

RogerRiggs Apr 2, 2025

liach Apr 15, 2025

david-beaumont Apr 24, 2025

liach Apr 24, 2025

david-beaumont Apr 24, 2025

liach Apr 24, 2025

david-beaumont Apr 24, 2025

8352750: Use jimage for preview enabled value classes #1409

Are you sure you want to change the base?

8352750: Use jimage for preview enabled value classes #1409

Conversation

david-beaumont commented Mar 26, 2025 • edited by openjdk bot Loading

Progress

Integration blocker

Issue

Reviewing

Choose a reason for hiding this comment

bridgekeeper bot commented Mar 26, 2025

openjdk bot commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-beaumont Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-beaumont commented Mar 26, 2025 •

edited by openjdk bot

Loading

david-beaumont Mar 26, 2025 •

edited

Loading