Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8275084: CDS warning when building with LOG=debug #5998

Closed
wants to merge 3 commits into from

Conversation

yminqi
Copy link
Contributor

@yminqi yminqi commented Oct 18, 2021

Hi, Please review
The generated SPECIES functions archived during dumping CDS archive. But some time rebuild issue warning
[2.325s][warning][cds] java.lang.ClassNotFoundException: java.lang.invoke.BoundMethodHandle$Species_LLLLL
Since we only care archiving functions generated in the four holder classes:
java.lang.invoke.Invokers$Holder, java.lang.invoke.LambdaForm$Holder, java.lang.invoke.DirectMethodHandle$Holder and java.lang.invoke.DelegatingMethodHandle$Holder
This patch removed the logging thus the archiving of SPECIES_RESOLVED functions in shared archive.

Tests: tier1,tier4

Thanks
Yumin


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8275084: CDS warning when building with LOG=debug

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5998/head:pull/5998
$ git checkout pull/5998

Update a local copy of the PR:
$ git checkout pull/5998
$ git pull https://git.openjdk.java.net/jdk pull/5998/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5998

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5998.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 18, 2021

👋 Welcome back minqi! 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.

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 18, 2021

/label add hotspot-runtime

@openjdk openjdk bot added the hotspot-runtime label Oct 18, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 18, 2021

@yminqi
The hotspot-runtime label was successfully added.

@yminqi yminqi marked this pull request as ready for review Oct 18, 2021
@openjdk openjdk bot added the rfr label Oct 18, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 18, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

This patch removed the logging thus the archiving of SPECIES_RESOLVED functions in shared archive.

I find it surprising that "logging" has a direct affect on archiving!

Overall it is not at all clear where the actual warnings were arising from, given that if you follow this "logging" code then these species classes should have been filtered out anyway:

static const char* filter[NUM_FILTER] = {"java.lang.invoke.Invokers$Holder",
                                         "java.lang.invoke.DirectMethodHandle$Holder",
                                         "java.lang.invoke.DelegatingMethodHandle$Holder",
                                         "java.lang.invoke.LambdaForm$Holder"};

static bool should_be_archived(char* line) {
  for (int k = 0; k < NUM_FILTER; k++) {
    if (strstr(line, filter[k]) != nullptr) {
      return true;
    }
  }
  return false;
}

@@ -139,9 +139,6 @@ static void traceSpeciesType(String cn, Class<?> salvage) {
if (TRACE_RESOLVE) {
System.out.println("[SPECIES_RESOLVE] " + cn + (salvage != null ? " (salvaged)" : " (generated)"));
}
if (CDS.isDumpingClassList()) {
CDS.traceSpeciesType("[SPECIES_RESOLVE]", cn);
Copy link
Member

@dholmes-ora dholmes-ora Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This CDS function seems to be unused now.

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 19, 2021

This patch removed the logging thus the archiving of SPECIES_RESOLVED functions in shared archive.

I find it surprising that "logging" has a direct affect on archiving!

Overall it is not at all clear where the actual warnings were arising from, given that if you follow this "logging" code then these species classes should have been filtered out anyway:

static const char* filter[NUM_FILTER] = {"java.lang.invoke.Invokers$Holder",
                                         "java.lang.invoke.DirectMethodHandle$Holder",
                                         "java.lang.invoke.DelegatingMethodHandle$Holder",
                                         "java.lang.invoke.LambdaForm$Holder"};

static bool should_be_archived(char* line) {
  for (int k = 0; k < NUM_FILTER; k++) {
    if (strstr(line, filter[k]) != nullptr) {
      return true;
    }
  }
  return false;
}

You are right, after excluded SPECIES_RESOLVE, there is no need to filter in hotspot code. I will remove all SPECIES_RESOLVE related code in CDS. Thanks

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 20, 2021

This patch removed the logging thus the archiving of SPECIES_RESOLVED functions in shared archive.

I find it surprising that "logging" has a direct affect on archiving!
Overall it is not at all clear where the actual warnings were arising from, given that if you follow this "logging" code then these species classes should have been filtered out anyway:

static const char* filter[NUM_FILTER] = {"java.lang.invoke.Invokers$Holder",
                                         "java.lang.invoke.DirectMethodHandle$Holder",
                                         "java.lang.invoke.DelegatingMethodHandle$Holder",
                                         "java.lang.invoke.LambdaForm$Holder"};

static bool should_be_archived(char* line) {
  for (int k = 0; k < NUM_FILTER; k++) {
    if (strstr(line, filter[k]) != nullptr) {
      return true;
    }
  }
  return false;
}

You are right, after excluded SPECIES_RESOLVE, there is no need to filter in hotspot code. I will remove all SPECIES_RESOLVE related code in CDS. Thanks

After internal discussion, decided not remove SPECIES_RESOLVE from class list, so they stilled keep shared. But remove them from classlist as regular class names. Those classes are not marked for un-shareable in flag bit since they have a source as regular class but they are not.

iklam
iklam approved these changes Oct 20, 2021
Copy link
Member

@iklam iklam left a comment

LGTM

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Seems reasonable.

Thanks,
David

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 21, 2021

@iklam @dholmes-ora Thanks for review!

@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 21, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@yminqi This pull request has not yet been marked as ready for integration.

@yminqi yminqi changed the title 8275084: CDS warning during incremental build 8275084: CDS warning when building with LOG=debug Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@yminqi 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:

8275084: CDS warning when building with LOG=debug

Reviewed-by: dholmes, 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 111 new commits pushed to the master branch:

  • 49f9d80: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key
  • bef8cf1: 8275714: G1: remove unused variable in G1Policy::transfer_survivors_to_cset
  • af14650: 8275569: Add linux-aarch64 to test-make profiles
  • 0761a4b: 8275688: Suppress warnings on non-serializable non-transient instance fields in DualPivotQuicksort
  • 3cb241a: 8275686: Suppress warnings on non-serializable non-transient instance fields in java.rmi
  • 3b0ce23: 8274888: Dump "-DReproduce=true" to the test VM command line output
  • d589b66: 8270380: Change the default value of the java.security.manager system property to disallow
  • e39bdc9: 8274714: Incorrect verifier protected access error message
  • 45ce06c: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST
  • 60cb27d: 8275426: PretouchTask num_chunks calculation can overflow
  • ... and 101 more: https://git.openjdk.java.net/jdk/compare/a16f2d0a3c326dd8b3b2133a9c170d998b7aa631...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 openjdk bot added the ready label Oct 21, 2021
@yminqi
Copy link
Contributor Author

@yminqi yminqi commented Oct 21, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

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

  • 49f9d80: 8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key
  • bef8cf1: 8275714: G1: remove unused variable in G1Policy::transfer_survivors_to_cset
  • af14650: 8275569: Add linux-aarch64 to test-make profiles
  • 0761a4b: 8275688: Suppress warnings on non-serializable non-transient instance fields in DualPivotQuicksort
  • 3cb241a: 8275686: Suppress warnings on non-serializable non-transient instance fields in java.rmi
  • 3b0ce23: 8274888: Dump "-DReproduce=true" to the test VM command line output
  • d589b66: 8270380: Change the default value of the java.security.manager system property to disallow
  • e39bdc9: 8274714: Incorrect verifier protected access error message
  • 45ce06c: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST
  • 60cb27d: 8275426: PretouchTask num_chunks calculation can overflow
  • ... and 101 more: https://git.openjdk.java.net/jdk/compare/a16f2d0a3c326dd8b3b2133a9c170d998b7aa631...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 21, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@yminqi Pushed as commit 7dd8237.

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

@yminqi yminqi deleted the jdk-8275084 branch Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated
3 participants