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

8273494: Zero: Put libjvm.so into "zero" folder, not "server" #5440

Closed

Conversation

shipilev
Copy link
Contributor

@shipilev shipilev commented Sep 9, 2021

Currently, the build system defaults the libjvm.so location to "server". This makes looking for libjvm.so awkward, see JDK-8273487 for example. We need to see if moving the libjvm.so to a proper location breaks anything.

Additional testing:

  • Linux x86_64 Zero build
  • Linux x86_64 Zero bootcycle-images
  • Linux x86_64 Zero tier1

Progress

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

Issue

  • JDK-8273494: Zero: Put libjvm.so into "zero" folder, not "server"

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5440

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 9, 2021

👋 Welcome back shade! 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
Copy link

@openjdk openjdk bot commented Sep 9, 2021

@shipilev The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • hotspot-runtime

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 pull request command.

@openjdk openjdk bot added hotspot-runtime build core-libs labels Sep 9, 2021
@shipilev shipilev marked this pull request as ready for review Sep 9, 2021
@openjdk openjdk bot added the rfr label Sep 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 9, 2021

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

This does what it intends but I can't approve it via a review as the issue is more about whether this should be done. The people who own/maintain/support Zero are the ones who should be directing this.

David

@@ -84,7 +84,7 @@ AC_DEFUN_ONCE([HOTSPOT_SETUP_JVM_VARIANTS],
fi

# All "special" variants share the same output directory ("server")
Copy link
Member

@dholmes-ora dholmes-ora Sep 9, 2021

Choose a reason for hiding this comment

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

I presume "zero" was a special variant? Are there any other variants remaining?

Copy link
Contributor Author

@shipilev shipilev Sep 9, 2021

Choose a reason for hiding this comment

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

Yes, there are at least "core" and "custom":

# All valid JVM variants
VALID_JVM_VARIANTS="server client minimal core zero custom"

Copy link
Member

@magicus magicus Sep 9, 2021

Choose a reason for hiding this comment

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

I think we should stop these as well from impersonating the server JVM. Preferably in the same fix, so we can remove all the special casing for "server" being anything else but server.

Copy link
Contributor Author

@shipilev shipilev Sep 9, 2021

Choose a reason for hiding this comment

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

Ok, I agree. Can I do a Zero-specific thing here (so that it is potentially cleanly backportable), and then handle the rest of the variants?

Copy link
Member

@magicus magicus Sep 9, 2021

Choose a reason for hiding this comment

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

Sure, that works for me.

Copy link
Contributor Author

@shipilev shipilev Sep 9, 2021

Choose a reason for hiding this comment

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

While we are at it, do core and custom even carry their weight? I cannot remember if I ever seen anyone using them. Maybe we should "just" drop those variants, and leave only "server, client, minimal, zero"?

Copy link
Member

@magicus magicus Sep 9, 2021

Choose a reason for hiding this comment

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

I don't know about "core". Last time it was up for discussion, some old-timer I've forgotten pointed out that it was useful for such-and-such. I'm not sure if the cost for keeping it is high enough to make the effort to figure out if anyone is going to miss it if we remove it.

As for "custom", it is not a real JVM configuration per se. Instead, it's more of a testing ground for the build system. Basically, the JVM "variants" are just named pre-defined sets of enabled JVM features. To test this properly, I introduced the "custom" variant which is just an empty set of JVM features from the start, so that individual features can be fully controlled. I don't really think anyone is using this in the "real world", but I'd like to keep it for testing purposes.

@@ -95,9 +95,9 @@ ifeq ($(call And, $(call isTargetOs, windows) $(call isTargetCpu, x86)), true)
endif
DEFAULT_CFG_VARIANT ?= server

# Any variant other than server, client or minimal is represented as server in
# Any variant other than server, client, minimal, or zero is represented as server in
Copy link
Member

@dholmes-ora dholmes-ora Sep 9, 2021

Choose a reason for hiding this comment

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

Are there any other variants now?

Copy link
Member

@magicus magicus left a comment

And as for @dholmes-ora comment: I'm not sure who "own" zero at this point in time. Aleksey has made a lot of the zero patches lately; does that not count? Are you thinking about any specific person that needs to weight in on this?

@@ -84,7 +84,7 @@ AC_DEFUN_ONCE([HOTSPOT_SETUP_JVM_VARIANTS],
fi

# All "special" variants share the same output directory ("server")
Copy link
Member

@magicus magicus Sep 9, 2021

Choose a reason for hiding this comment

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

I think we should stop these as well from impersonating the server JVM. Preferably in the same fix, so we can remove all the special casing for "server" being anything else but server.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 9, 2021

@magicus every "port" in OpenJDK is supposed to have a clear owner and support system. Zero has been somewhat lacking in that area but there were enough people to keep it surviving. Now I'm not so sure. Does Zero have an active user community? Developer community? If so they are the ones who need to assess this change.

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 10, 2021

@magicus every "port" in OpenJDK is supposed to have a clear owner and support system. Zero has been somewhat lacking in that area but there were enough people to keep it surviving. Now I'm not so sure. Does Zero have an active user community? Developer community? If so they are the ones who need to assess this change.

FWIW, Zero had fallen into Red Hat hands for support. The official lead (if you look at Census) is Gary Benson, who is not active in this project anymore. Since then, it was mostly supported by RH folks (like me) with contributions from Debian, Tencent, Huawei, Alibaba -- mostly because all of us have arch ports that do not have full-blown Server VMs yet.

I have put things in motion to claim the leadership more formally. I do have to note, though, that over the last few years of me whipping Zero into shape, this is the first time anyone asked the formal governance question, which must tell us something about how much we care about ownership formalities here ;)

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 10, 2021

It isn't the "formal governance" I'm concerned about, more about the folk who use/rely on Zero being the ones to evaluate the impact of a change like this. People, like myself, who do not use Zero in any way cannot evaluate whether this change is appropriate - it's that simple. :)

@gnu-andrew
Copy link
Member

@gnu-andrew gnu-andrew commented Sep 10, 2021

@magicus every "port" in OpenJDK is supposed to have a clear owner and support system. Zero has been somewhat lacking in that area but there were enough people to keep it surviving. Now I'm not so sure. Does Zero have an active user community? Developer community? If so they are the ones who need to assess this change.

FWIW, Zero had fallen into Red Hat hands for support. The official lead (if you look at Census) is Gary Benson, who is not active in this project anymore. Since then, it was mostly supported by RH folks (like me) with contributions from Debian, Tencent, Huawei, Alibaba -- mostly because all of us have arch ports that do not have full-blown Server VMs yet.

For clarity, Gary was a part of the Red Hat Java team at the time, so it has always been a Red Hat project. At the time, the only JIT ports in OpenJDK were x86, x86-64 and SPARC, so the other architectures Red Hat supported (ppc, ppc64, s390, s390x) needed some way to at least build and run to allow OpenJDK packages to be shipped, even if end users desiring greater performance used some other JDK.

As OpenJDK has gained further JIT ports, the use of Zero has declined in response. For example, we can build OpenJDK 17 on every architecture we need without using Zero, so it's not going to receive that kind of "you broke our build" testing any more. It is still used for 8u & 11u, and is definitely worth keeping alive to help bootstrap any new ports.

So, in short, the owner has always been and remains Red Hat, even if the individual personnel have changed. The formal project represents a time when it was maintained outside the mainline OpenJDK, so there has been no need to update that for a long time, as any JDK project committer can make changes to the code in the main JDK project.

I have put things in motion to claim the leadership more formally. I do have to note, though, that over the last few years of me whipping Zero into shape, this is the first time anyone asked the formal governance question, which must tell us something about how much we care about ownership formalities here ;)

Most of the OpenJDK committers at Red Hat have pushed Zero fixes at some time, myself included, as need has arisen. This has the first time this has been raised to my knowledge.

Regarding this change itself, I think it's fine for trunk, where there's time to shake out any issues for OpenJDK 18, but I wouldn't want to backport it. These kind of changes tend to throw up things only when someone's application breaks, and, at least if that's a new JDK version, that's somewhat expected.

Copy link
Member

@magicus magicus left a comment

I'm changing my response to "approve". I agree that we can change zero only in this patch to facilitate backporting, and fix the remaining odds and ends in a separate PR.

@magicus
Copy link
Member

@magicus magicus commented Sep 14, 2021

And also let me be clear that I see no reason to delay pushing this any further. Everyone who has any vested interest in zero has had ample chance to comment on the PR.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2021

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

8273494: Zero: Put libjvm.so into "zero" folder, not "server"

Reviewed-by: ihse, sgehwolf

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

  • 0f31d0f: 8273373: Zero: Cannot invoke JVM in primordial threads on Zero
  • fe89dd3: 8271254: javac generates unreachable code when using empty semicolon statement
  • 8974b95: 8273730: WorkGangBarrierSync constructor unused
  • 1d3eb14: 8273635: Attempting to acquire lock StackWatermark_lock/9 out of order with lock tty_lock/3
  • 31667da: 8273491: java.util.spi.LocaleServiceProvider spec contains statement that is too strict
  • ed7789d: 8256977: Bump minimum GCC from 5.x to 6 for JDK
  • 5bfd043: 8273497: building.md should link to both md and html
  • 3884580: 8273597: Rectify Thread::is_ConcurrentGC_thread()
  • f527289: 8273639: tests fail with "assert(_handle_mark_nesting > 1) failed: memory leak: allocating handle outside HandleMark"
  • 1d2458d: 8266550: C2: mirror TypeOopPtr/TypeInstPtr/TypeAryPtr with TypeKlassPtr/TypeInstKlassPtr/TypeAryKlassPtr
  • ... and 54 more: https://git.openjdk.java.net/jdk/compare/aa9311182ae88312a70b18afd85939718415b77c...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 Sep 14, 2021
Copy link
Contributor

@jerboaa jerboaa left a comment

Looks fine to me.

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 14, 2021

I'll be happy to integrate as long as @dholmes-ora has no objections.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 15, 2021

@shipilev no objections from me. I will leave it to the Zero developers to worry, or not, about the Zero users.

Cheers,
David

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 15, 2021

Thank you all!

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

Going to push as commit 8fbcc82.
Since your change was applied there have been 74 commits pushed to the master branch:

  • 92c30c9: 8273599: Remove cross_threshold method usage around GC
  • 02af541: 8273605: VM Exit does not abort concurrent mark
  • febcc72: 8273366: [testbug] javax/swing/UIDefaults/6302464/bug6302464.java fails on macOS12
  • 1017a2c: 8273135: java/awt/color/ICC_ColorSpace/MTTransformReplacedProfile.java crashes in liblcms.dylib with NULLSeek+0x7
  • 6cf70f5: 8273638: javax/swing/JTable/4235420/bug4235420.java fails in GTK L&F
  • e66bf47: 8273414: ResourceObj::operator delete should handle nullptr in debug builds
  • 16c3ad1: 8272574: C2: assert(false) failed: Bad graph detected in build_loop_late
  • e7ab372: 8273641: (bf) Buffer subclasses documentation contains template strings
  • 22a7191: 8273040: Turning off JpAllowDowngrades (or Upgrades)
  • 394ebc8: 8270553: Tests should not use (real, in-use, routable) 1.1.1.1 as dummy IP value
  • ... and 64 more: https://git.openjdk.java.net/jdk/compare/aa9311182ae88312a70b18afd85939718415b77c...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Sep 15, 2021

@shipilev Pushed as commit 8fbcc82.

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

@shipilev shipilev deleted the JDK-8273494-zero-build-server-zero branch Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build core-libs hotspot-runtime integrated
5 participants