8373114: Redundant MethodCounters in the preimage generated by training run

[ashu-mehra]

Details are mentioned in the bug report and this mail thread https://mail.openjdk.org/pipermail/leyden-dev/2025-December/002843.html
After this patch the number of MethodCounters in the preimage are much less than before this patch. Also the number of MethodCounters in the preimage is the same as in the final AOTCache (these numbers are obtained using -Xlog:aot=trace).

Before this patch for the training run:

[18.610s][debug  ][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[18.610s][debug  ][aot         ] MethodCounters      :        0          0   0.0 |    22471    1438144   6.2 |    22471    1438144   2.6

Before this patch for the assembly phase:

[6.680s][debug][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[6.680s][debug][aot         ] MethodCounters      :        0          0   0.0 |     8588     549632   2.5 |     8588     549632   1.0

After this patch for the training run:

[49.371s][debug  ][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[49.371s][debug  ][aot         ] MethodTrainingData  :        0          0   0.0 |     4658     521696   2.1 |     4658     521696   0.9

After this patch for the assembly phase:

[12.580s][debug][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[12.580s][debug][aot         ] MethodTrainingData  :        0          0   0.0 |     4658     521696   2.1 |     4658     521696   0.9

---

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-8373114: Redundant MethodCounters in the preimage generated by training run (Enhancement - P4)

Reviewers

• Ioi Lam (@iklam - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28670

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back asmehra! 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]

@ashu-mehra 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:

8373114: Redundant MethodCounters in the preimage generated by training run

Reviewed-by: iklam

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 165 new commits pushed to the master branch:

• 9862f8f: 8373513: C2: Move ProjNode::other_if_proj() to IfProjNode
• 39306d7: 8373800: Remove ScopedValueBindingsResolver
• 5e7ae28: 8373677: Clear text HttpServer connection could fail fast if receiving SSL ClientHello
• ... and 162 more: https://git.openjdk.org/jdk/compare/2735140147b159d3a3238804f221db4f835ef744...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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]

@ashu-mehra 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.

[openjdk]

@ashu-mehra Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[mlbridge]

Webrevs

• 02: Full - Incremental (9bbb4789)
• 01: Full - Incremental (b6bbd8c8)
• 00: Full (f741349f)

[ashu-mehra]

I did a force-push to fix the bug id in the commit message.

[iwanowww]

I'm not an expert in CDS, but the fix doesn't look right. It breaks Metadata traversal CDS relies on. Instead, if you don't want to keep around MethodCounters not linked to MTDs, break the link from Method (Method::_method_counters) before dumping pre-image and restore it from archived TrainingDataSet during assembly phase.

[ashu-mehra]

I'm not an expert in CDS, but the fix doesn't look right. It breaks Metadata traversal CDS relies on. Instead, if you don't want to keep around MethodCounters not linked to MTDs, break the link from Method (Method::_method_counters) before dumping pre-image and restore it from archived TrainingDataSet during assembly phase.

Right, this is not the correct fix. It breaks the Method->MethodCounters link for all the methods. We need to preserve the links for the MethodCounters that get added through MTD.

[ashu-mehra]

Right, this is not the correct fix. It breaks the Method->MethodCounters link for all the methods. We need to preserve the links for the MethodCounters that get added through MTD.

hmm, I think I am wrong here again. We don't need to preserve the Method->MCS link. We only need the MTD->MCS link.

[ashu-mehra]

I looked at the code again and unless I missed something it looks like the MCS from MTD are not used at all. I don't see their usage either in mainline or in leyden premain branch. Why do we even care to store MCS at all then? @veresov can you please comment about this?

[veresov]

They are not used right now but @vnkozlov is going to use them for the A4 recompilation strategy.

[vnkozlov]

They are not used right now but @vnkozlov is going to use them for the A4 recompilation strategy.

yes, I need it for A4 entry counter.

Also I thought MC is used to cache MTD pointer:
https://github.com/openjdk/leyden/blob/master/src/hotspot/share/oops/trainingData.cpp#L120

[veresov]

Yes, but caching is a runtime thing it's not really need in the archive. But for your work we probably do need it.

[ashu-mehra]

Okay, I updated the fix. Idea is the same as before - add MethodCounters reachable from MethodTrainingData, not from Method. But the new fix relies on updating ArchiveBuilder::get_follow_mode to add the filter instead of changing Metadata traversal. The filter relies on the assumption that only the MethodCounters that are associated with MethodTrainingData need to be stored in the AOT cache.
This additional filtering was not enough as I noticed number of MCS were still more than number of MTD. This is because some new MCS are created during the assembly phase and they get linked to the existing MTD. This situation was fixed by updating MethodTrainingData::make() to avoid doing any lookup in the assembly phase when the JVM is outside AOT safepoint. This makes sense because we shouldn't be using training data for compilations that happen outside of AOT safe point in the assembly phase.

Another approach I thought of was to add the filtering in ArchiveBuilder::get_follow_mode such that only the MethodCounters referenced by MTDs are stored in the AOTCache. It is possible to get the enclosing MetaspaceObj for the MetaspaceClosure::Ref passed to get_follow_mode but I couldn't find an easy way to get the type of the enclosing object, which makes it difficult to check if the MethodCounters being added came from Method or MTD.

[veresov]

What's the percentage of the memory usage in the archive do you typically see attributed to MCs?

[ashu-mehra]

Before the patch with mainline:
In pre-image:

[19.021s][debug  ][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[19.021s][debug  ][aot         ] MethodCounters      :        0          0   0.0 |    22485    1439040   6.2 |    22485    1439040   2.6
[19.021s][debug  ][aot         ] MethodTrainingData  :        0          0   0.0 |     4998     519792   2.2 |     4998     519792   0.9

In final AOTCache:

[6.655s][debug][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %  
[6.655s][debug][aot         ] MethodCounters      :        0          0   0.0 |     8535     546240   2.5 |     8535     546240   1.0
[6.655s][debug][aot         ] MethodTrainingData  :        0          0   0.0 |     4998     519792   2.3 |     4998     519792   1.0

After the new fix, I see following stats:

In pre-image:

[12.397s][debug  ][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[12.397s][debug  ][aot         ] MethodCounters      :        0          0   0.0 |     5440     348160   1.6 |     5440     348160   0.6
[12.397s][debug  ][aot         ] MethodTrainingData  :        0          0   0.0 |     4985     518440   2.3 |     4985     518440   1.0

In final AOTCache:

[7.452s][debug][aot         ]                         ro_cnt   ro_bytes     % |   rw_cnt   rw_bytes     % |  all_cnt  all_bytes     %
[7.452s][debug][aot         ] MethodCounters      :        0          0   0.0 |     4942     316288   1.4 |     4942     316288   0.6
[7.452s][debug][aot         ] MethodTrainingData  :        0          0   0.0 |     4985     518440   2.3 |     4985     518440   1.0

So the size of MethodCounters is 2.6% (before fix) vs 0.6% (after fix) in the preimage, and 1% (before fix) vs 0.6% (after fix) in the AOTCache.

[iklam]

Another approach I thought of was to add the filtering in ArchiveBuilder::get_follow_mode such that only the MethodCounters referenced by MTDs are stored in the AOTCache. It is possible to get the enclosing MetaspaceObj for the MetaspaceClosure::Ref passed to get_follow_mode but I couldn't find an easy way to get the type of the enclosing object, which makes it difficult to check if the MethodCounters being added came from Method or MTD.

Hi Ashutosh, I've created a patch for finding the type of the enclosing obj:

iklam@cef9437

Could you try to see if that works for you?

Thanks

[ashu-mehra]

Another approach I thought of was to add the filtering in ArchiveBuilder::get_follow_mode such that only the MethodCounters referenced by MTDs are stored in the AOTCache. It is possible to get the enclosing MetaspaceObj for the MetaspaceClosure::Ref passed to get_follow_mode but I couldn't find an easy way to get the type of the enclosing object, which makes it difficult to check if the MethodCounters being added came from Method or MTD.

Hi Ashutosh, I've created a patch for finding the type of the enclosing obj:

iklam@cef9437

Could you try to see if that works for you?

@iklam thanks for the changes. This is indeed what I wanted and updated the patch with the following change:

          address enclosing_obj = ref->enclosing_obj();
          if (ref->enclosing_obj_msotype() == MetaspaceObj::MethodTrainingDataType) {
            return make_a_copy;
          }

But it seems like this is not sufficient. With this approach number of MethodCounters are only 1142 while the MTDs are around 4k. This is because in ArchiveBuilder::gather_one_source_obj the MCs that are first reached from Method get stored in the _src_obj_table with _follow_mode=set_to_null. Subsequent visits to such MCs through MTDs are ignored.

[iklam]

Another approach I thought of was to add the filtering in ArchiveBuilder::get_follow_mode such that only the MethodCounters referenced by MTDs are stored in the AOTCache. It is possible to get the enclosing MetaspaceObj for the MetaspaceClosure::Ref passed to get_follow_mode but I couldn't find an easy way to get the type of the enclosing object, which makes it difficult to check if the MethodCounters being added came from Method or MTD.

Hi Ashutosh, I've created a patch for finding the type of the enclosing obj:
iklam@cef9437
Could you try to see if that works for you?

@iklam thanks for the changes. This is indeed what I wanted and updated the patch with the following change:

          address enclosing_obj = ref->enclosing_obj();
          if (ref->enclosing_obj_msotype() == MetaspaceObj::MethodTrainingDataType) {
            return make_a_copy;
          }

But it seems like this is not sufficient. With this approach number of MethodCounters are only 1142 while the MTDs are around 4k. This is because in ArchiveBuilder::gather_one_source_obj the MCs that are first reached from Method get stored in the _src_obj_table with _follow_mode=set_to_null. Subsequent visits to such MCs through MTDs are ignored.

Currently set_to_null means that the referenced object should be always be excluded (with all subsequent references). Maybe we can change the meaning to "set the current reference to null, but allow the referenced object to be included via other references". If we make this change, it will be easier to handle similar situations in the future.

I am not sure if there's a compatibility issue -- probably not. Currently, we have never had a situation where the follow-mode of a referenced object is sometimes set_to_null and sometimes make_a_copy.

However, the implementation will be more complicated, as we have to remember the decision on each reference (currently the decision is remembered on the referenced object).

Maybe we should go with your first approach for now, and experiment with the second approach in a later RFE.

[iklam]

LGTM