8344140: Refactor the discovery of AOT cache artifacts #22291

iklam · 2024-11-21T05:27:43Z

This is a clean up after the initial integration for JEP 483

Merged 3 loops into 1 for discovering the classes and oops that should be stored in the AOT cache. About 250 lines of code are deleted.
Added comments in aotArtifactFinder.hpp to describe the logic, which is much simpler than before.
Enum classes are no longer unconditionally AOT-initialized -- an Enum class is AOT-initialized only if we find an archived oop of this type.

Note to reviewers:

One consequence of this refactoring is that archived oops are now discovered (by heapShared.cpp) before the metadata are copied (by ArchiveBuilder). There are some functions that depend on the old order (metadata were copied before oop discovery), so I had to shuffle them around. Examples are Modules::check_archived_module_oop(), or the new code in Klass::remove_java_mirror().

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8344140: Refactor the discovery of AOT cache artifacts (Enhancement - P4)

Reviewers

Ashutosh Mehra (@ashu-mehra - Committer) Review applies to c5ed9578
Calvin Cheung (@calvinccheung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22291

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22291.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-21T05:28:35Z

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

openjdk · 2024-11-21T05:28:56Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8344140: Refactor the discovery of AOT cache artifacts

Reviewed-by: ccheung, asmehra

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-21T05:29:38Z

@iklam The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-21T05:32:52Z

Webrevs

…consolidate-aot-artifact-gathering-discovery

…-discovery

bridgekeeper · 2024-12-19T08:17:45Z

@iklam This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

ashu-mehra · 2025-01-07T22:47:06Z

@iklam nice refactoring/cleanup! The comments and the logic in AOTArtifactFinder makes it a lot easier to understand the algorithm now.

ashu-mehra · 2025-01-07T22:52:16Z

Comment in AOTClassInitializer::can_archive_initialized_mirror() refers to HeapShared::find_all_aot_initialized_classes() which does not exist anymore:

  // When CDSConfig::is_initing_classes_at_dump_time() is enabled,
  // HeapShared::find_all_aot_initialized_classes() finds the classes of all
  // heap objects that are reachable from HeapShared::_run_time_special_subgraph,
  // and mark these classes as aot-inited.

And this comment at line 263 in aotClassInitializer.cpp also needs to be updated as the enum classes are not unconditionally AOT-initialized:

      // above we selected all enums; we must include their super as well
      {"java/lang/Enum"},

ashu-mehra · 2025-01-07T22:47:35Z

src/hotspot/share/cds/metaspaceShared.cpp

@@ -543,6 +544,7 @@ class StaticArchiveBuilder : public ArchiveBuilder {

  virtual void iterate_roots(MetaspaceClosure* it) {
    FileMapInfo::metaspace_pointers_do(it);
+    AOTArtifactFinder::all_cached_classes_do(it);
    SystemDictionaryShared::dumptime_classes_do(it);


There is some duplicatoin of work in AOTArtifactFinder::all_cached_classes_do() and SystemDictionaryShared::dumptime_classes_do(). all_cached_classes_do() operates on _all_cached_classes which is built using SystemDictionaryShared::_dumptime_table. So the iteration over _dumptime_table in SystemDictionaryShared::dumptime_classes_do() seems redundant.

The call to SystemDictionaryShared::dumptime_classes_do() is still necessary. It does more than just discovering the classes. It also walks the DumpTimeClassInfo with

info.metaspace_pointers_do(it);

which will walk the loader constraints, etc.

Right, missed that. Thanks for pointing that out.

iklam · 2025-01-08T02:38:37Z

Comment in AOTClassInitializer::can_archive_initialized_mirror() refers to HeapShared::find_all_aot_initialized_classes() which does not exist anymore:
  // When CDSConfig::is_initing_classes_at_dump_time() is enabled,
  // HeapShared::find_all_aot_initialized_classes() finds the classes of all
  // heap objects that are reachable from HeapShared::_run_time_special_subgraph,
  // and mark these classes as aot-inited.
And this comment at line 263 in aotClassInitializer.cpp also needs to be updated as the enum classes are not unconditionally AOT-initialized:
      // above we selected all enums; we must include their super as well
      {"java/lang/Enum"},

I fixed the comments and removed the entry for java/lang/Enum

…-discovery

calvinccheung

Some minor comments below.

calvinccheung · 2025-01-13T21:27:17Z

src/hotspot/share/cds/aotArtifactFinder.hpp

+//
+// Note1: See TODO comments in HeapShared::archive_object() for exceptions to this rule.
+//
+// Note2: The scanning of Java objects is done in heapShared.cpp. Please see calls into the HeapShared class


Note2 isn't referenced by the comment above. Perhaps line 62 should reference it?

calvinccheung · 2025-01-13T21:28:46Z

src/hotspot/share/cds/heapShared.cpp

+    Klass* k = obj->klass();
+    if (k->is_instance_klass()) {
+      // Whenever we see a non-array Java object of type X, we mark X to be aot-initialized.
+      // This ensures that during the production run, whenever Java code seens a cached object


typo: s/seens/sees

calvinccheung · 2025-01-13T21:31:03Z

src/hotspot/share/cds/heapShared.cpp

+        // We can't rerun <clinit> of enum classes (see cdsEnumKlass.cpp) so
+        // we must store them as AOT-initialized.
+        AOTArtifactFinder::add_aot_inited_class(InstanceKlass::cast(k));
+      } else if (subgraph_info == _dump_time_special_subgraph) {


Can this be an "or" condition rather than "else if" since the same function is called for either on of the conditions?

calvinccheung · 2025-01-13T21:33:24Z

src/hotspot/share/cds/heapShared.cpp

    // Don't add the Klass containing the sub-graph to it's own klass
    // initialization list.
    return;
  }

-  if (buffered_k->is_instance_klass()) {
+  if (orig_k->is_instance_klass()) {
+    InstanceKlass* ik = InstanceKlass::cast(orig_k);


Should this be inside a "#if ASSERT" since ik is used only in the assert statements?

calvinccheung

Updates look good.

…-discovery

iklam · 2025-01-15T04:31:57Z

Thanks @ashu-mehra and @calvinccheung for the review.
I've tested against tiers 1~4 and builds-tier5. No new failures.
/integrate

openjdk · 2025-01-15T04:32:48Z

Going to push as commit be1cdd9.
Since your change was applied there have been 10 commits pushed to the master branch:

973c630: 8342466: Improve API documentation for java.lang.classfile.attribute
9782bfd: 8347620: Shenandoah: Use 'free' tag for free set related logging
35be4a4: 8347173: java/net/DatagramSocket/InterruptibleDatagramSocket.java fails with virtual thread factory
36b7abd: 8225763: Inflater and Deflater should implement AutoCloseable
d6d45c6: 8303884: jlink --add-options plugin does not allow GNU style options to be provided
0ee6ba9: 8347596: Update HSS/LMS public key encoding
ec2aaaa: 8326236: assert(ce != nullptr) failed in Continuation::continuation_bottom_sender
02d2493: 8347613: Remove leftover doPrivileged call in Currency test: CheckDataVersion.java
10d08db: 8346142: [perf] scalability issue for the specjvm2008::xml.validation workload
9b1bed0: 8290043: serviceability/attach/ConcAttachTest.java failed "guarantee(!CheckJNICalls) failed: Attached JNI thread exited without being detached"

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-15T04:32:53Z

@iklam Pushed as commit be1cdd9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

iklam added 5 commits November 19, 2024 08:51

8344140: Consolidate AOT cache code for artifact discovery

ed6bd0a

Removed unnecessary code

26cd958

More clean up

4681daa

tmp

7b85362

more clean up

be0ef2e

openjdk bot added the rfr Pull request is ready for review label Nov 21, 2024

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 21, 2024

iklam added 3 commits November 23, 2024 16:27

Fixe 32-bit builds; removed unused function

29946dd

Merge branch 'master' of https://github.com/openjdk/jdk into 8344140-…

e6b6a04

…consolidate-aot-artifact-gathering-discovery

Merge branch 'master' into 8344140-consolidate-aot-artifact-gathering…

31a7e9b

…-discovery

ashu-mehra reviewed Jan 7, 2025

View reviewed changes

@ashu-mehra comments

c5ed957

ashu-mehra approved these changes Jan 8, 2025

View reviewed changes

iklam added 2 commits January 11, 2025 11:04

Merge branch 'master' into 8344140-consolidate-aot-artifact-gathering…

204ca82

…-discovery

Updated copyright

e246276

calvinccheung reviewed Jan 13, 2025

View reviewed changes

@calvinccheung comments

6275013

calvinccheung approved these changes Jan 14, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 14, 2025

Merge branch 'master' into 8344140-consolidate-aot-artifact-gathering…

07cacbd

…-discovery

openjdk bot added the integrated Pull request has been integrated label Jan 15, 2025

openjdk bot closed this Jan 15, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 15, 2025

TimPushkin mentioned this pull request Mar 19, 2025

8352394: [CRaC] AOT class linking cannot archive jdk.internal.crac.Core$Priority openjdk/crac#213

Draft

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8344140: Refactor the discovery of AOT cache artifacts #22291

8344140: Refactor the discovery of AOT cache artifacts #22291

iklam commented Nov 21, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 21, 2024

openjdk bot commented Nov 21, 2024 •

edited

Loading

openjdk bot commented Nov 21, 2024

mlbridge bot commented Nov 21, 2024 •

edited

Loading

bridgekeeper bot commented Dec 19, 2024

ashu-mehra commented Jan 7, 2025 •

edited

Loading

ashu-mehra commented Jan 7, 2025

ashu-mehra Jan 7, 2025

iklam Jan 8, 2025

ashu-mehra Jan 8, 2025

iklam commented Jan 8, 2025

calvinccheung left a comment

calvinccheung Jan 13, 2025

iklam Jan 14, 2025

calvinccheung Jan 13, 2025

iklam Jan 14, 2025

calvinccheung Jan 13, 2025

iklam Jan 14, 2025

calvinccheung Jan 13, 2025

iklam Jan 14, 2025

calvinccheung left a comment

iklam commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

8344140: Refactor the discovery of AOT cache artifacts #22291

8344140: Refactor the discovery of AOT cache artifacts #22291

Conversation

iklam commented Nov 21, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 21, 2024

openjdk bot commented Nov 21, 2024 • edited Loading

openjdk bot commented Nov 21, 2024

mlbridge bot commented Nov 21, 2024 • edited Loading

Webrevs

bridgekeeper bot commented Dec 19, 2024

ashu-mehra commented Jan 7, 2025 • edited Loading

ashu-mehra commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Jan 8, 2025

calvinccheung left a 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

calvinccheung left a comment

Choose a reason for hiding this comment

iklam commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

openjdk bot commented Jan 15, 2025

iklam commented Nov 21, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 21, 2024 •

edited

Loading

mlbridge bot commented Nov 21, 2024 •

edited

Loading

ashu-mehra commented Jan 7, 2025 •

edited

Loading