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

8134540: Much nearly duplicated code for PerfMemory support #2037

Closed
wants to merge 4 commits into from

Conversation

@hseigel
Copy link
Member

@hseigel hseigel commented Jan 11, 2021

Please review this change to merge the Unix-like PerfMemory_*.cpp source files into a single PerfMemory_posix.cpp source file.

The changes were tested on Mach5 tiers 1-5 on Linux, Windows, and Mac OS, and cross-builds were done for the S390, arm32, ppc, and x86 platforms.

Thanks, Harold


Progress

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

Issue

  • JDK-8134540: Much nearly duplicated code for PerfMemory support ⚠️ Issue is not open.

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 11, 2021

👋 Welcome back hseigel! 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 openjdk bot added the rfr label Jan 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2021

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

  • hotspot-runtime

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
Copy link

@mlbridge mlbridge bot commented Jan 11, 2021

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 12, 2021

To make this change easier to review, the below webrev shows the differences between the new perfMemory_posix.cpp file and the old perfMemory_aix.cpp, perfMemory_bsd,cpp, and perfMemory_linux.cpp files.

http://cr.openjdk.java.net/~hseigel/perfMemory_changes/

(This webrev is just to show the differences. It will not be part of the push.)

Thanks, Harold

Copy link
Contributor

@coleenp coleenp left a comment

Looks great!

@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

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

8134540: Much nearly duplicated code for PerfMemory support

Reviewed-by: coleenp, dholmes

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

  • 82adfb3: 8134540: Much nearly duplicated code for PerfMemory support
  • a9519c8: 8259924: GitHub actions fail on Linux x86_32 with "Could not configure libc6:i386"
  • 139f5d3: 8259035: Comments for load order of hsdis should be updated
  • bd81ccf: 8259957: Build failure without C1 Compiler after JDK-8258004
  • dfee7b8: 8259511: java/awt/Window/MainKeyWindowTest/TestMainKeyWindow.java failed with "RuntimeException: Test failed: 20 failure(s)"
  • 14ce8f1: 8259870: zBarrier.inline.hpp should not include javaClasses.hpp
  • a1a851b: Merge
  • bb0821e: 8258643: [TESTBUG] javax/swing/JComponent/7154030/bug7154030.java failed with "Exception: Failed to hide opaque button"
  • cd25bf2: 8259574: SIGSEGV in BFSClosure::closure_impl
  • d5ca3b3: 8259641: C2: assert(early->dominates(LCA)) failed: early is high enough
  • ... and 131 more: https://git.openjdk.java.net/jdk/compare/8dfc77bfa669a29d275c202ce83422ccb6853717...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 Jan 13, 2021
Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Harold,

I had an initial look. A few comments below.

The amount of Linux specific code is a bit concerning for a shared implementation.

Thanks,
David

src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jan 14, 2021

Hi Harold,

Sorry I forgot to say thanks for tackling this - deduplication is good! But maybe we can refactor the Linux specific bits more cleanly by pushing helper functions into the os namespace, just for Linux and then calling this via LINUX_ONLY.

Thanks,
David

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 15, 2021

Hi David,
Thanks for your helping improve this change. This updated change attempts to address your comments. I reduced the amount of Linux specific bits by using LINUX_ONLY and by moving function get_namespace_pid(int vmid) to os_linux.cpp. If need be, I'd like to address the remaining LINUX specific code in a separate RFE.

Could you please review this updated commit?

Thanks, Harold

Copy link
Member

@tstuefe tstuefe left a comment

Hi Harold,

thanks a lot for this! I think most of the AIX peculiarities are just bitrots (us missing changes to other platforms and forgetting to side-merge). Thankfully this becomes posix shared code now.

Thanks, Thomas

src/hotspot/os/posix/os_posix.inline.hpp Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Show resolved Hide resolved
src/hotspot/os/posix/perfMemory_posix.cpp Outdated Show resolved Hide resolved
@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 15, 2021

Hi Thomas,

Thanks for looking at this change, especially from an AIX perspective. Please review the changes and comments in this latest commit and see if additional changes are needed.

Thanks, Harold

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Jan 16, 2021

Looks good to me. I have currently no access to an AIX machine, a test build would have to wait until next week. If you are eager, feel free to push though, should anything break we will fix it then.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Harold,

Thanks for the updates. Still some scope for future refactoring for Linux specific container stuff, but this looks good enough to me.

Thanks,
David

@tstuefe
Copy link
Member

@tstuefe tstuefe commented Jan 19, 2021

Hi Harold,

AIX build breaks with your change because you have a typo there (do you use vi?)

With this fix it builds:

--- a/src/hotspot/os/posix/perfMemory_posix.cpp
+++ b/src/hotspot/os/posix/perfMemory_posix.cpp
@@ -342,7 +342,7 @@ static DIR *open_directory_secure(const char* dirname) {
   }

   // Check to make sure fd and dirp are referencing the same file system object.
-  if (!is_same_fsobject(fd, AIX_ONLY(dirp->dd_fdi) NOT_AIX(dirfd(dirp)))) {
+  if (!is_same_fsobject(fd, AIX_ONLY(dirp->dd_fd) NOT_AIX(dirfd(dirp)))) {
     // The directory is not secure.
     os::close(fd);
     os::closedir(dirp);

No tests ran yet because of this, but I'll reschedule the AIX tests.

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 19, 2021

Hi Thomas,
Thanks for finding the AIX_ONLY issue. I"ll fix it before pushing the change.
Harold

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 19, 2021

Thanks Coleen, David, and Thomas for reviewing this change!

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jan 19, 2021

@hseigel Warning! Your commit did not result in any changes! No push attempt will be made.

Copy link
Contributor

@coleenp coleenp left a comment

Good.

@openjdk openjdk bot added ready and removed ready labels Jan 19, 2021
@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 19, 2021

Thanks Coleen, David, and Thomas for reviewing this change!

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jan 19, 2021

@hseigel Warning! Your commit did not result in any changes! No push attempt will be made.

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 19, 2021

Thanks Coleen, David, and Thomas for reviewing this change!

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jan 19, 2021

@hseigel Warning! Your commit did not result in any changes! No push attempt will be made.

@hseigel
Copy link
Member Author

@hseigel hseigel commented Jan 19, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jan 19, 2021

@hseigel Warning! Your commit did not result in any changes! No push attempt will be made.

@edvbld
Copy link
Member

@edvbld edvbld commented Jan 20, 2021

Pushed as commit 82adfb3, but the bots could not close this pull request due to GitHub throwing an 500 Internal Server Error. I'm closing this pull request manually.

@edvbld edvbld closed this Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants