search

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.28k stars 721 forks source link

Discussion: Allow multiple -Xjit args #12503

Open dsouzai opened 3 years ago

dsouzai commented 3 years ago

Basically every component in OpenJ9 allows the user to specify the option prefex multiple times (for example the GC: -Xgc:verbose -Xgc:concurrentScavenge will result in both options getting applied). Additionally, having the ability to specify the option more than once without fear of the second overriding the first in its entirety is pretty standard behaviour; what we do is actually highly unintuitive and can cause all sorts of unintended behaviour.

I realize that there maybe some tests that depend on our existing behaviour, but the right way forward is to fix the tests. In terms of having this work in the compiler, it is actually incredibly easy - we just read all the -Xjit strings and concatenate into one big string before passing it over to the bulk of the options processing; probably <20 lines of code.

mstoodle commented 3 years ago

what we do is actually highly unintuitive and can cause all sorts of unintended behaviour.

Except for the fact that we've been consistently doing it this way for 20 years :) . Are you sure another group of people (our existing users, for example) won't find the reverse to be highly unintuitive and cause all sorts of unintended behaviour ?

Scripts anywhere (not just tests, but any kind of Java deployment) can be (accidentally? intentionally) relying on multiple JIT options not being merged. Making a change like this could expose behaviour changes ranging from command-line conflicts (JVM won't start) to crashes to silent functional change to silent performance impacts.

All that doesn't necessarily mean we shouldn't do it, but it does mean 1) we would need to communicate this kind of change very loudly and well in advance of the change actually happening, and 2) we should be prepared to accept and help people to deal with inevitable fallout.

Is there evidence that more people are affected/confused by the way it works than will be affected/confused by the way you think it should work?

JamesKingdon commented 3 years ago

Definitely on the service side we are continually fighting with Xjit options not being correctly applied because the customers expect to be able to use multiple copies and have them sensibly merged.

klangman commented 3 years ago

We constantly have to put warnings in our messages to customers about using -Xjit because they mess it up so often. They mess it up an ways that any reasonable administrator would expect to work.

dsouzai commented 3 years ago

Except for the fact that we've been consistently doing it this way for 20 years :) .

I'm not the first one to bring this up; every time the topic has been brought up in the past, the argument has always been that existing things depend on the behaviour (which I think is a pretty weak argument given that the compiler options were never documented and were always a "use at your own risk" thing). My point of this is, just because we've done it consistently doesn't mean it's intuitive; it just means we've been forced (for one reason or another) to continue to deal with the unintuitive behaviour.

Are you sure another group of people (our existing users, for example) won't find the reverse to be highly unintuitive and cause all sorts of unintended behaviour ?

Basically every dev has been burned by this at least once - it's something we always have to keep in the back of our minds.

Scripts anywhere (not just tests, but any kind of Java deployment) can be (accidentally? intentionally) relying on multiple JIT options not being merged. Making a change like this could expose behaviour changes ranging from command-line conflicts (JVM won't start) to crashes to silent functional change to silent performance impacts.

All that doesn't necessarily mean we shouldn't do it, but it does mean 1) we would need to communicate this kind of change very loudly and well in advance of the change actually happening, and 2) we should be prepared to accept and help people to deal with inevitable fallout.

There are ways to help mitigate this; for example a super ugly temp solution can be to have an env var that reverts to the old behaviour to help people who are seeing their set up not work anymore (as I said, the solution is so easy on the compiler side that it can literally be put into an if statement)

fjeremic commented 3 years ago

If we concatenate options what happens if someone writes:

  1. -Xjit:disableIdiomRecognition -Xjit:enableIdiomRecognition
  2. -Xjit:{MyClass.foo()V}(traceFull,log=foo.trace) -Xjit:{MyClass.foo()V}()

For comparison if you write -Xdump:none -Xdump:system+java -Xdump:none -Xdump:jit:events=vmstop only a JitDump will be generated at vmstop event.

dsouzai commented 3 years ago
  1. I imagine the same as if someone did -Xjit:disableIdiomRecognition,enableIdiomRecognition - hopefully we enable it lol
  2. I don't know what happens if you did -Xjit:{method}(...),{method}()...knowing our options processing, we probably crash...
klangman commented 3 years ago

Scripts anywhere (not just tests, but any kind of Java deployment) can be (accidentally? intentionally) relying on multiple JIT options not being merged. Making a change like this could expose behaviour changes ranging from command-line conflicts (JVM won't start) to crashes to silent functional change to silent performance impacts.

In the past we would have made a change of this nature in a new major release. That is not so easy to do in our current development model so I think we would just have to bite the bullet and take some pain to arrive a a better place in the future.

dsouzai commented 3 years ago
  1. I don't know what happens if you did -Xjit:{method}(...),{method}()...knowing our options processing, we probably crash...

Shockingly, does not crash! It will open a trace log but it won't write to it. Colour me surprised.

0xdaryl commented 3 years ago

I think any change to the public-facing options behaviour needs to be done carefully with a transition plan in mind. I am open-minded to change if it makes sense, but there has to be a plan that includes a timeline and communication to existing consumers of OpenJ9.

I don't think the OpenJ9 project has a documented policy and process for deprecating external features or modifying externally-visible default behaviour. Perhaps it should have one, as it would take the "policy" out of discussions like the one in this issue and keep the discussion focused on the technical merits of the proposal.

There may be other aspects of the JIT command-line options that could stand for some modernization as well. Prior to this issue, I know there has been some interest expressed in evaluating the state of the JIT command-line options and their effectiveness. For example, is the syntax too verbose, should + and - be used to enable and disable, should there be one JIT option string or individual -XX options, is there a better syntax for specifying numerical ranges than regexes, etc.? The behaviour change proposed in this issue could be considered as part of this broader effort. In fact, it may be cleaner and less disruptive to change default behaviours in one fell swoop rather than through multiple changes on different timelines.

There has been talk in the past about unifying the command-line processing in the JIT with the other VM components and OMR. If there is interest in exploring this or even a JIT-focused effort, perhaps someone would like to champion that cause. :-)

klangman commented 3 years ago

Maybe we need to invent a new option and deprecate -Xjit. We could even stop supporting -Xjit on everything newer then JavaXX then at some point it will be dead code that can be safely removed.

dsouzai commented 3 years ago

The behaviour change proposed in this issue could be considered as part of this broader effort. In fact, it may be cleaner and less disruptive to change default behaviours in one fell swoop rather than through multiple changes on different timelines.

Ideally, yes, but we also have to be pragmatic; we only have to look at the progress of the new OMR Options processing improvements to see that any massive change is going to a) take a long time to implement and test, and b) take a long time to actually get merged.

That's why I find @klangman's suggestion a really good compromise. The semantics can be:

  1. If the new JIT/AOT options are specified, -Xjit/-Xaot are ignored
  2. The new JIT/AOT options allow multiple args
  3. The new JIT/AOT options are initially identical to -Xjit/-Xaot.

When the time comes that we have the resources to develop the +/- semantics, we can have a transition where the new JIT/AOT options can handle both +/- & existing options (but not both at the same time); this can continue for a few releases until we finally pull the plug and allow only +/-. This approach may take longer, but it will have less impact on consumers and it won't be one massive change that gets dropped (complete with subtle bugs).

dsouzai commented 2 years ago

fyi, latest example of users confused that only the latest -Xjit option takes into effect: https://github.com/eclipse-openj9/openj9-docs/issues/873

dsouzai commented 2 years ago

Another example https://github.com/eclipse-openj9/openj9/issues/14157

dsouzai commented 2 weeks ago

https://github.com/eclipse-openj9/openj9/pull/16133#issuecomment-2399782557

JamesKingdon commented 44 minutes ago This problem cropped up again, and since this issue forms part of the documentation trail, adding the note that Paul C checked and it looks like MergeCompilerOptions was available as of ibm sdk 8.0.8.0