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 720 forks source link

Merge -Xjit/-Xaot opts by default in JDK 21 #16289

Open dsouzai opened 1 year ago

dsouzai commented 1 year ago

Addresses: https://github.com/eclipse-openj9/openj9/issues/12503

https://github.com/eclipse-openj9/openj9/pull/16133 added the ability to merge multiple -Xjit or -Xaot options. After talking to service engineers, they suggested that it would be good to have this become the default in some future release. I brought this up in the Nov 8 2022 OpenJ9 Community Call, and it was recommended that the JDK 21 release would be a good place to do this because it is a LTS release; the JDK 20 docs can state that the next release will change the default.

Does this sound like a good plan? As per discussion in the call, If JDK 21 doesn't work, then the next release to do this would be the next LTS (~24~ 25).

Risks

Benefits

Notes

[1] https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-823531265 [2] https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-823537553 [3] https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-1005102329 [4] https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-1231896468

@vijaysun-omr @0xdaryl @mstoodle fyi @klangman @JamesKingdon

pshipton commented 1 year ago

We still have time to deprecate in Java 19, assuming OpenJ9 does a Java 19 release (which is not certain). It is 25 which is the next LTS after 21.

vijaysun-omr commented 1 year ago

Is it worth summarizing the risks and benefits again (or do we go through the lengthy discussions in linked issue(s)) for actually switching the default ? Is there more than one way to go about such a switch in which case that may be a set of alternatives that people think about ? Just wondering if we need to focus more on the "what" we are agreeing to (rather than "how" to do it) at this stage. I don't have any particular position on it yet, but having that summary might nudge me one way or another.

dsouzai commented 1 year ago

Is it worth summarizing the risks and benefits again (or do we go through the lengthy discussions in linked issue(s)) for actually switching the default ? Is there more than one way to go about such a switch in which case that may be a set of alternatives that people think about ? Just wondering if we need to focus more on the "what" we are agreeing to (rather than "how" to do it) at this stage.

Updated description with information to this end.

pshipton commented 1 year ago

Is there a way to turn off previous options so you can start fresh? Something like -Xjit:none (similar to -Xdump:none). This is used when adding a new option to the end of the command line (via an environment variable, web form, test harness/grinder), but the previous command line is not easily modified.

dsouzai commented 1 year ago

Is there a way to turn off previous options so you can start fresh? Something like -Xjit:none (similar to -Xdump:none).

Technically yes, but only if -Xjit:none is a standalone option; if it were something like -Xjit:none,... it would be quite non-trivial to deal with given the options processing framework. As such, if someone were to do -Xjit:none,... then we'd end up hitting an error because the compiler would complain that there is no none option.

FWIW, the reason we could make it work if it was only a standalone option is because we can search for -Xjit:none and then only look for -Xjit / -Xaot options seen from that point on; none wouldn't actually get processed by the options processing framework.

vijaysun-omr commented 1 year ago

Given this https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-823545034 about -Xdump it would appear that not all other major command line options (apart from -Xjit) work consistently either.

I do certainly see the value from a service engineer perspective (the 4 links that you pasted). Especially when a new set of options is specified by the service engineer to (say) disable an optimization or trace a method or profile a run, it can be frustrating to see the option not get applied as they had wanted. Are these examples for switching the default though OR for having the ability to guarantee that the new options get picked up (as would happen with your prior change to introduce the option that a service engineer could use when they sent new options to the customer) ?

I saw that there was a question about conflicting options https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-823545034 and while the answer talked about having the same behavior as if the user had specified conflicting options in the same -Xjit command, the fact remains that a) that wasn't the old behavior and b) combining distinct -Xjit commands increases the likelihood of having conflicting options (is a user really going to have conflicting options in the same -Xjit command ? it seems unlikely to me). I guess I am thinking about the change in behavior that a customer (not one of the internal devs) would get, not because that customer knew what they were doing necessarily (i.e. they may not have known anything about -Xjit precedence/combining rules) but more because its just a change in behavior for them after N years potentially of working a certain way that they were happy with. Having said that, such a customer case could be resolved by giving them -XX:-MergeCompilerOptions to revert to the old behaviour but this can only be done after the service team has already been engaged and a certain amount of cost has been incurred in understanding the problem. So, there is a scenario in which it increases service cost as well.

At the end of the day, I'm fine deferring to the service team on this question since they have more of the direct customer experience to say whether the change to the default will be a net win or not. It seems they believe it to be a net win, in which case my comments above may just be food for thought.

pshipton commented 1 year ago

Has it been proposed to leave -Xjit/-Xaot alone and have new options that combine? i.e. -Xjitc/-Xaotc. This way the behaviour doesn't change and when using the new options the rules are understood.

The last -Xjit option wins, but if there are following -Xjitc options, they combine with themselves and with any previous -Xjit option.

dsouzai commented 1 year ago

So, there is a scenario in which it increases service cost as well.

Yeah that's a fair point. For the most part, I've opened this issue based on conversations with service folk, but I personally can't speak to the cost saved by having it default vs the cost incurred due to the scenario you mentioned. I'll leave it to @klangman and/or @JamesKingdon to comment as they would be in the best position to speak to the tradeoffs.

Has it been proposed to leave -Xjit/-Xaot alone and have new options that combine? i.e. -Xjitc/-Xaotc. This way the behaviour doesn't change and when using the new options the rules are understood.

Yeah, in https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-824245530 (and subsequent comment) there was discussion about having new compiler options that by default merge. I had opened https://github.com/eclipse-openj9/openj9/pull/16133 as a stop gap to at least have the ability to merge options without intending to make it the default; I only went down this path because of the previously mentioned conversations. However, if having -Xjitc and -Xaotc options is a more preferred way of going forward, then I'm ok with that too.

The last -Xjit option wins, but if there are following -Xjitc options, they combine with themselves and with any previous -Xjit option.

If we're going to introduce new options, I don't think we should complicate things by having -Xjit and -Xjitc combine; if the latter is seen, then the former is ignored (even if we have a situation like -Xjit:.. -Xjitc:... -Xjit:....

JamesKingdon commented 1 year ago

I think pretty much any approach will have outlier cases where the behaviour isn't ideal, so from a service perspective improving the most common scenario is the goal, while trying to avoid the worst of the possible failure modes. I've never seen a case with a customer running more than one Xjit option and relying on the last one wins behaviour, so that's not a scenario I'm overly worried about. The cases where we run into problems are invariably where the customer has an existing single Xjit option in place from a previous case and the potential confusion that arises from having to manually combine it with any new options we need for the current investigation. Often these pre-existing Xjit options were applied years ago by someone else in their organisation and sometimes even finding where it is defined can be challenging. On a somewhat related note, I'm seeing a recent trend of JVM command line options being duplicated, presumably as a result of there being so many ways of supplying them. I could see this trend causing us problems in service without some form of the capability of merging the Xjit options, so a big thank you to Irwin for championing this cause. Backtracking a little, Vijay's point about whether it needs to be the default behaviour or simply an available choice via -XX:+MergeCompilerOptions is valid. We could manage either way, but from the service perspective it will make our lives simpler to have it as the default since it is one less thing to have to get the customer to set correctly. The engineers on the customer side are often under a lot of pressure in high stress situations and anything we can do to make their lives simpler and potentially avoid another iteration of data collection is a big win.

dsouzai commented 1 year ago

However, if having -Xjitc and -Xaotc options is a more preferred way of going forward, then I'm ok with that too.

I will mention though that in the case where we do add new options, @0xdaryl's point in https://github.com/eclipse-openj9/openj9/issues/12503#issuecomment-824197619 does have merit in that, if we there is an eventual desire to make the compiler options better in general (+/- semantics or whatever), then we would need to add yet another means of specifying jit/aot options.

0xdaryl commented 1 year ago

I think it would help to make this decision if we knew strategically how we wanted the JIT options to be architected and how they would behave. That way we would know whether any adjustments we make to the existing behaviour are helping or hurting that evolution (assuming an evolution is even desired).

However, at this point I think we have far more real examples of where the lack of combining options has either fooled customers or hindered our efforts to provide service than users expecting the rightmost option to win. I’m willing to concede that a change in behaviour is required provided it is staged properly.

I think the JIT should begin to emit a message in JDK 19/20 when multiple -Xjit’s are present on the command-line about how this behaviour is expected to change in the next LTS going forward. A JIT option should also be provided to suppress this warning message should it be interfering with a user.

The problem with the message warning approach, however, is that I’m doubtful many customers wishing to adopt JDK 21 will be following the evolution JDK 19 -> JDK 20 -> JDK 21 and instead moving from JDK 11/17 to JDK 21 (someone can correct me if I’m wrong). This is based on anecdotal reports I’ve heard about low download rates of feature releases. Meaning that most will just experience the new behaviour without seeing any of the warning messages emitted.

Now, I’m not sure how common multiple -Xjit’s are on the command-line but I doubt it is a significant part of all deployments. If that’s the case, then changing the behaviour and emitting a warning message about the new behaviour might still be warranted even in JDK 21.

dsouzai commented 1 year ago

The problem with the message warning approach, however, is that I’m doubtful many customers wishing to adopt JDK 21 will be following the evolution JDK 19 -> JDK 20 -> JDK 21 and instead moving from JDK 11/17 to JDK 21 (someone can correct me if I’m wrong). This is based on anecdotal reports I’ve heard about low download rates of feature releases. Meaning that most will just experience the new behaviour without seeing any of the warning messages emitted.

Does it make sense then to have say the warning message and documentation about the behaviour change in JDK 21, and then actually do the change in JDK25? It's a slower process, but the warning message will be displayed in JDK 21-24, and then JDK 25 just has the change along with the appropriate message in the release docs. I don't know you all feel like this is way too slow though.

klangman commented 1 year ago

I think the JIT should begin to emit a message in JDK 19/20 when multiple -Xjit’s are present on the command-line about how this behaviour is expected to change in the next LTS going forward. A JIT option should also be provided to suppress this warning message should it be interfering with a user.

I am sure that all customers and products that adopt a new major version of the JDK will do a ton of testing prior to deployment and therefore I think a note in the readme would be sufficient. Such a change as this should never be done in a point release but on the flip side, customers should expect changes of this nature on major releases.

I'll add my support for this change in general, I proactively remind people about the fact that only one "-Xjit" option will take effect and despite this we still have the occasional case were the warring is not heeded. In addition, I frequently get questions from customers and support about how to merge two options. This change should improve the usability of the JIT in my opinion.

0xdaryl commented 1 year ago

If we're talking about JDK 25 (likely 3 years out) I don't think we should be implementing an interim solution at that time. We should have a clear picture of how we'd like the JIT options to appear and behave by then (possibly some unifying story with other components of the JVM), and then implementing that.

pshipton commented 1 year ago

Java 21 - Sept 2023 (OpenJDK schedule, OpenJ9 may be delayed) Java 25 - Sept 2025

dsouzai commented 1 year ago

@0xdaryl do you still want there to be a message printed out in JDK19/20, or given Kevin's comment is updating the documentation sufficient?