8382584: [lworld] Mandatory pre-loading still present in systemDictionary.cpp#2409
8382584: [lworld] Mandatory pre-loading still present in systemDictionary.cpp#2409caspernorrbin wants to merge 2 commits into
Conversation
|
👋 Welcome back cnorrbin! A progress list of the required criteria for merging this PR into |
|
@caspernorrbin 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: 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 216 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@coleenp) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
openjdk
Bot
changed the title
Webrevs
|
| // A false return means that the class didn't load for other reasons than an exception. | ||
| bool check = preload_from_null_free_field(ik, class_loader, sig, field_index, CHECK_NULL); | ||
| if (!check) { | ||
| ik->set_shared_loading_failed(); |
There was a problem hiding this comment.
Can we safely drop this failure marking?
There was a problem hiding this comment.
It's used for regular shared class loading so I don't think we can drop this shared_loading_failed marker.
There was a problem hiding this comment.
Didn't realise that one was needed. I've re-added it, along with an edit to try_preload_from_loadable_descriptors so we can see if we failed or not. It now also has a new parameter that decides if failing means we call shared_loading_failed .
| // A false return means that the class didn't load for other reasons than an exception. | ||
| bool check = preload_from_null_free_field(ik, class_loader, sig, field_index, CHECK_NULL); | ||
| if (!check) { | ||
| ik->set_shared_loading_failed(); |
There was a problem hiding this comment.
It's used for regular shared class loading so I don't think we can drop this shared_loading_failed marker.
3 participants
Hi everyone,
This change removes the remaining mandatory preloading path for null-restricted instance fields when loading classes from CDS. JDK-8380053 already changed normal class loading so null-restricted field types are no longer loaded just because the field is null-restriced. Instead, the preloading is speculative and driven by the
LoadableDescriptorattribute. The CDS path still had the older special case inSystemDictionary::preload_from_null_free_field, which could force loading of null-restricted field types.This fix removes that CDS-only preload helper and routes non-static field signatures through the existing
LoadableDescriptorpath instead. This makes loading follow the same model as inClassFileParser::fetch_field_classes()with speculative loading.Testing:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2409/head:pull/2409$ git checkout pull/2409Update a local copy of the PR:
$ git checkout pull/2409$ git pull https://git.openjdk.org/valhalla.git pull/2409/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2409View PR using the GUI difftool:
$ git pr show -t 2409Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2409.diff
Using Webrev
Link to Webrev Comment