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

8253900: SA: wrong size computation when JVM was built without AOT #358

Closed
wants to merge 1 commit into from

Conversation

@jrziviani
Copy link
Contributor

@jrziviani jrziviani commented Sep 25, 2020

TestInstanceKlassSize was failing because, for PowerPC, the following code (instanceKlass.cpp) always compiles to return false;

bool InstanceKlass::has_stored_fingerprint() const {
#if INCLUDE_AOT
  return should_store_fingerprint() || is_shared();
#else
  return false;
#endif
}

However, in hasStoredFingerprint()@InstanceKlass.java the condition shouldStoreFingerprint() || isShared(); is always evaluated and may return true (AFAIK isShared() returns true). Such condition adds 8 bytes in the getSize()@InstanceKlass.java causing the failure in TestInstanceKlassSize:

public long getSize() { // in number of bytes
  ...
  if (hasStoredFingerprint()) {
    size += 8; // uint64_t
  }
  return alignSize(size);
}

Considering these tests are failing for PowerPC only (based on ProblemList.txt), my solution checks if hasStoredFingerprint() is running on a PowerPC platform. I decided to go this way because there is no existing flag informing whether AOT is included or not and creating a new one just to handle the PowerPC case seems too much.

This patch is an attempt to fix https://bugs.openjdk.java.net/browse/JDK-8230664


Progress

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

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (3/3 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8253900: SA: wrong size computation when JVM was built without AOT

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/358/head:pull/358
$ git checkout pull/358

@bridgekeeper bridgekeeper bot added the oca label Sep 25, 2020
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 25, 2020

Hi @jrziviani, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jrziviani" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

Loading

@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Sep 25, 2020

/covered

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 25, 2020

@jrziviani The following labels will be automatically applied to this pull request: hotspot-gc serviceability.

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

Loading

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 25, 2020

Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 29, 2020

Webrevs

  • 04: Full - Incremental (8e9d136)
  • 03: Full (5c08095fb924b786afbead306d90aa0cdc785248)
  • 02: Full (bc555b2ecec945adf4e0767d3d36802b30db30f5)
  • 01: Full - Incremental (8fc1ab6ff9a8b2bfbdf9beee6e1f7b0449dff7d5)
  • 00: Full (1d4ad8241689a4052d5ad4e17d985c4f56ce8ac1)

Loading

Copy link
Contributor

@plummercj plummercj left a comment

Can you please update the JBS issue to accurately describe what the underlying cause is. It incorreclty states that it is 8-byte vs 16-byte aligment.

I'd prefer that you added someting like VM.hasAOT(). This will fix the problem for other CPU ports that may be in a similar situation and also ensure correctness when configure a build with --disable-aot.

Loading

@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Sep 29, 2020

@plummercj
Thank you for your review!

Can you please update the JBS issue to accurately describe what the underlying cause is. It incorreclty states that it is 8-byte vs 16-byte aligment.

Unfortunately, I don't have write access to the bug system. I'll check if the original reporter would mind adding it for me.

I'd prefer that you added someting like VM.hasAOT(). This will fix the problem for other CPU ports that may be in a similar situation and also ensure correctness when configure a build with --disable-aot.

Sure, I'll work on it. I'll update this PR as soon as I finish the tests.

Thank you!

Loading

@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Sep 30, 2020

@plummercj Hello! I implemented hasAOT() by using an existing variable in methodCounters that exists only if AOT is enabled.

ppc64le

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSize.java
                                                         1     1     0     0   
==============================
TEST SUCCESS

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java
                                                         1     1     0     0   
==============================
TEST SUCCESS

x86-64

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSize.java
                                                         1     1     0     0   
==============================
TEST SUCCESS

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
   jtreg:test/hotspot/jtreg/serviceability/sa/TestInstanceKlassSizeForInterface.java
                                                         1     1     0     0   
==============================
TEST SUCCESS```

Loading

Copy link
Contributor

@plummercj plummercj left a comment

Changes look good.

Loading

Copy link
Contributor

@sspitsyn sspitsyn left a comment

LGTM

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Oct 1, 2020

I think this fix deserves a new JBS issue. It doesn't resolve the rounding problem described in JDK-8230664. It fixes an additional issue. The rounding problem may reoccur when other class layout changes are done.

Loading

@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Oct 1, 2020

I think this fix deserves a new JBS issue. It doesn't resolve the rounding problem described in JDK-8230664. It fixes an additional issue. The rounding problem may reoccur when other class layout changes are done.

Totally agreed. But, would you mind to create the ticket for me, please? Then, I change the commit header/message.
Thank you!!

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Oct 1, 2020

Hi Jose,
here's your new bug: https://bugs.openjdk.java.net/browse/JDK-8253900

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Oct 1, 2020

I think this fix deserves a new JBS issue. It doesn't resolve the rounding problem described in JDK-8230664. It fixes an additional issue. The rounding problem may reoccur when other class layout changes are done.

Does it still need to be problem listed due to this issue? I guess it may or may not be an issue based on the current size InstanceKlass, so probably best to keep in problem listed.

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Oct 1, 2020

If these tests are currently passing with Jose's fix, I suggest to comment them out in the problem list with a note that we may need to disable them again because of JDK-8230664.
This way we can test the functionality Jose is fixing until then. And we'll see when the issue comes up again.

Loading

@jrziviani jrziviani changed the title 8230664: Fix TestInstanceKlassSize for PowerPC 8253900: SA: wrong size computation when JVM was built without AOT Oct 1, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 1, 2020

⚠️ @jrziviani the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout JDK-8230664
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 1, 2020

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

8253900: SA: wrong size computation when JVM was built without AOT

Reviewed-by: cjplummer, sspitsyn

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

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 (@plummercj, @sspitsyn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

Loading

@openjdk openjdk bot added the ready label Oct 1, 2020
@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Oct 1, 2020

If these tests are currently passing with Jose's fix, I suggest to comment them out in the problem list with a note that we may need to disable them again because of JDK-8230664.
This way we can test the functionality Jose is fixing until then. And we'll see when the issue comes up again.

Done! Please, check if you're fine with this:

+# The solution to bug JDK-8253900 seems to fix tests TestInstanceKlassSize and
+# TestInstanceKlassSizeForInterface. However, while JDK-8230664 is not resolved,
+# these tests may be disabled again if necessary.
+# serviceability/sa/TestInstanceKlassSize.java 8230664 linux-ppc64le,linux-ppc64
+# serviceability/sa/TestInstanceKlassSizeForInterface.java 8230664 linux-ppc64le,linux-ppc64

Thank you Martin!

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Oct 1, 2020

If these tests are currently passing with Jose's fix, I suggest to comment them out in the problem list with a note that we may need to disable them again because of JDK-8230664.
This way we can test the functionality Jose is fixing until then. And we'll see when the issue comes up again.

Done! Please, check if you're fine with this:

+# The solution to bug JDK-8253900 seems to fix tests TestInstanceKlassSize and
+# TestInstanceKlassSizeForInterface. However, while JDK-8230664 is not resolved,
+# these tests may be disabled again if necessary.
+# serviceability/sa/TestInstanceKlassSize.java 8230664 linux-ppc64le,linux-ppc64
+# serviceability/sa/TestInstanceKlassSizeForInterface.java 8230664 linux-ppc64le,linux-ppc64

I don't believe there are any other cases were we comment out a test in a problem list. I think it would be best just to remove it completely from ProblemList.txt. JDK-8230664 will still be filed. I suggest maybe adding a comment there saying that the test was removed from the problem list when JDK-8253900 was fixed, but should be re-added if JDK-8230664 starts to reproduce again.

Loading

@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Oct 1, 2020

... I suggest maybe adding a comment there saying that the test was removed from the problem list when JDK-8253900 was fixed, but should be re-added if JDK-8230664 starts to reproduce again.

I like this idea because a simple grep will be enough to understand what happened. Just pushed my branch, any issue just let me know.

Thank you Chris!

Loading

@plummercj
Copy link
Contributor

@plummercj plummercj commented Oct 1, 2020

... I suggest maybe adding a comment there saying that the test was removed from the problem list when JDK-8253900 was fixed, but should be re-added if JDK-8230664 starts to reproduce again.

I like this idea because a simple grep will be enough to understand what happened. Just pushed my branch, any issue just let me know.

Hi Ziviani,

My suggestion was to put the comment in the CR. I really don't think there should be any comment in ProblemList.txt. This is not an uncommon situation. Tests are often removed from the problem list simply because they have not been seen for a while (and we want to determine if the bug is still an issue) or because failures are rare, or possibly even so hard to reproduce that we want to eventually trigger a failure as part of regular testing to help with failure analysis. We have not added comments in the past to document these problem list removals.

Loading

The code hasStoredFingerprint() at InstanceKlass.java is not considering
AOT disabled at compilation time, like has_stored_fingerprint() at
instanceKlass.cpp does. Such difference can cause TestInstanceKlassSize
failures because all objects will have an extra 8-bytes.

TestInstanceKlassSize and TestInstanceKlassSizeForInterface were removed
from ProblemList.txt because it cannot be reproduced after this change,
but, if JDK-8230664 starts to reproduce again, these tests can be
disabled.
@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Oct 2, 2020

My suggestion was to put the comment in the CR. I really don't think there should be any comment in ProblemList.txt. This is not an uncommon situation. Tests are often removed from the problem list simply because they have not been seen for a while (and we want to determine if the bug is still an issue) or because failures are rare, or possibly even so hard to reproduce that we want to eventually trigger a failure as part of regular testing to help with failure analysis. We have not added comments in the past to document these problem list removals.

It's clear now. Sorry for the confusion. :-). I removed the tests from problemlist again and add a comment in my commit message. Any git blame will easily find it.

Hope I did it right now :-D

Thanks Chris!

Loading

Copy link
Contributor

@plummercj plummercj left a comment

Looks good.

Loading

Copy link
Contributor

@sspitsyn sspitsyn left a comment

LGTM

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Oct 2, 2020

Added comment to JDK-8230664. Test results look good on our side, too.

Loading

@jrziviani
Copy link
Contributor Author

@jrziviani jrziviani commented Oct 5, 2020

/integrate

Loading

@openjdk openjdk bot added the sponsor label Oct 5, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 5, 2020

@jrziviani
Your change (at version 8e9d136) is now ready to be sponsored by a Committer.

Loading

@TheRealMDoerr
Copy link
Contributor

@TheRealMDoerr TheRealMDoerr commented Oct 9, 2020

/sponsor

Loading

@openjdk openjdk bot closed this Oct 9, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 9, 2020

@TheRealMDoerr @jrziviani Since your change was applied there have been 95 commits pushed to the master branch:

  • 2bc8bc5: 8254265: s390 and linux 32 bit builds broken
  • 4f9a1ff: 8254073: Tokenizer improvements (revised)
  • 9cecc16: 8254244: Some code emitted by TemplateTable::branch is unused when running TieredCompilation
  • a95590d: 8254285: G1: Remove "What is this about" comment in G1CollectedHeap.cpp
  • 0230781: 8254175: Build no-pch configuration in debug mode for submit checks
  • b9873e1: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing
  • a2f6519: 8233685: Test tools/javac/modules/AddLimitMods.java fails
  • 70be8c7: 8253965: Delete the outdated java.awt.PeerFixer class
  • ced46b1: 8254190: [s390] interpreter misses exception check after calling monitorenter
  • 5351ba6: 8254262: jdk.test.lib.Utils::createTemp* don't pass attrs
  • ... and 85 more: https://git.openjdk.java.net/jdk/compare/3c4e824aa5fd99337759fa5000f5673ab2430750...master

Your commit was automatically rebased without conflicts.

Pushed as commit b1448da.

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

Loading

@openjdk openjdk bot removed the rfr label Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants