Openj9: Building JITaaS in master branch

[mpirvu]

We plan to port the JIT-as-a-Service (JITaaS) code from its own branch into master as outlined in https://github.com/eclipse/openj9/issues/6072
Integrating the JITaaS code can be done using one of the two approaches summarized below. I would like people to weigh in and express their preferences/concerns so that a decision can be taken soon.

Approach 1: Use #ifdef to protect JITaaS specific code; use a build configuration flag to compile-in the JITaaS code
Advantages:

• Fully transparent for developers not interested in JITaaS; they don't need to change anything
• No risk for non-JITaaS builds
• No size increases for non-JITaaS builds

Disadvantages:

• Sometimes, non-JITaaS code changes can cause issues for JITaaS builds
• Consumability of the JITaaS technology is hindered because users may have to re-build the JDK and create their own Docker containers

To address the consumability aspect we propose to have adoptopenjdk produce (and test) builds with JITaaS code (just for Java8 in the beginning). openliberty has containers based on adoptopenjdk:8-jre-openj9, hence anybody building on top of such a container could use JITaaS functionality just by enabling an environment variable.

Approach 2: JITaaS code is compiled-in unconditionally; hence, both openj9 builds at adoptopenjdk and IBM SDK for Java will include this code.
Advantages:

• Liberty docker images get JITaaS support out of the box
• It's easier on developers that want to test or play with JITaaS

Disadvantages:

• Cumbersome for developers not interested in JITaaS because they need to have the protobuf library compiled/installed on their system
• protobuf library needs be present at runtime which means that we need to copy libprotobuf.so into SDK (or statically link its code)
• docker image sizes will increase slightly: JITaaS code increases JIT dll size by 2.3MB; statically linking protobuf library adds another 3.7MB; libprotobuf.so is 27MB

For minimum disruption I am supporting Approach 1.

[dsouzai]

How much code would be guarded by the ifdef? Are the majority of the changes in their own isolated files, or will there be a lot of ifdef regions in say CompilationThread.cpp?

[fjeremic]

I'm in favor of Approach 2 because Approach 1 also requires a rebuild of the JIT (so likely the JVM) during test phases if we want to test JITaaS, not to mention the constant breakages because code is not being compiled, and unlikely to be tested during PR testing as it would require triggering special PR test builds.

[mpirvu]

How much code would be guarded by the ifdef?

While we tried as much as possible to put JITaaS code in separate files, some existing files (like CompilationThread.cpp) have lots of small JITaaS snippets.
It's astonishing, but we had to touch ~90 files to include JITaaS support.

[charliegracie]

Can we take advantage of autotools/CMake detection to check for the JITaaS requirements (protobuf, etc.)? If they are found then JITaaS is compiled in. If they are not found JITaaS is not compiled in. Also there should be options to configure/CMake to allow users to force it on / off which override the auto detection. In the case where it is forced on and the auto detection can not find required pre-reqs the build fails at configure time.

This would allow the code to be in by default as long as the build machine is capable of building it. All PR and nightly build machines should be capable of building it so it would help ensure the code is pretty much always tested.

[dsouzai]

While we tried as much as possible to put JITaaS code in separate files, some existing files (like CompilationThread.cpp) have lots of small JITaaS snippets.

Hm, in that case I'd lean towards Approach 2 for much the same reasons as @fjeremic . I'm remembering a time when we had code guarded by TR_MEM_STATS, and how we'd run into build issues when we really needed it. I suppose it's not really a fair comparison, because as long as there are people working on JITaaS and we have a nightly JITaaS build, the ifdefs would mostly work. But from a long term planning pov, it's probably best for it to not be ifdefs.

[mstoodle]

There are several kinds of builds here that are allowed to use different approaches:
A. Developer build (note that's all developers including non-JIT devs and even non-contributor devs)
B. PR build
C. Final build at AdoptOpenJDK

If we do Approach 2, then all of those use cases will be forced to satisfy the new protobuf dependency even if they have no interest in anything JITaaS. I therefore support option #1 (with a configure/cmake option) to include JITaaS code in the build that I recommend we use selectively across those three scenarios.

A: Both developers who want and don't want JITaaS can easily do their work with a minimum of additional fuss (either you configure/cmake with the option or you don't).

B: I suggest that PR builds include at least builds both with and without (so our build machines will need to have protobuf installed). That way both kinds of developers in A are supported. Yes, that's more machine resource requirement, but it won't be every platform and we shouldn't have to duplicate every test even on the platforms where JITaaS is supported .

C: I suggest that we recommend AdoptOpenJDK binaries include JITaaS support by default (though only activated by command-line option / environment variable). So no additional binary variants at AdoptOpenJDK to confuse people. JITaaS features become available to anyone once they pick up the new builds.

Irrelevant since there aren't any #ifdefs at the moment:
~Note that keeping the current `#ifdef' is a decision that can be changed later if everyone falls in love with JITaaS. Putting them back in could be more painful once the code evolves (for some level of "more" which is admittedly probably small to begin with).~

[keithc-ca]

Does including JITaaS come with any security considerations? For example, does it listen for compile requests over the network by default?

[mpirvu]

does it listen for compile requests over the network by default?

When configured to run in server mode (-XX:JITaaSServer) it does listen for compile requests over the network. Encryption is supported, but optional

[andrewcraik]

Past experience with having #ifdef'd code has been that it tends to rot because it isn't compiled by default and PRs will tend to break it by accident imposing a cost on the owners of the code to continually maintain it. With autotools / cmake we could just detect the availability and grab the dependency as necessary - it shouldn't be a big deal to get it. If that is not feasible then we have no choice but to have the #ifdefs but I would still want to see it on in as many builds as possible. In general I want support to be a default and for the tooling to make this as painless as possible.

I would suggest that if we go with some kind of CMake/autotools enablement where it isn't guaranteed the JaaS support is on that we add a message to either -version or -Xinternalversion or similar to report the compilation time support and the runtime support status of the feature.

[mstoodle]

The point was to do PR builds both with and without the #ifdefs to avoid the rot effect, for what it's worth.

[andrewcraik]

Do the projects have enough machine resources to support that and testing of both? I guess I am coming down as more in favour of option 2 because of code simplicity, lower testing burden and the fact we should be able to make the acquisition of protobuff "easy" with the autotools/CMake scripts (at least in theory).

[DanHeidinga]

Approach 1: Use #ifdef to protect JITaaS specific code; use a build configuration flag to compile-in the JITaaS code

I support option 1 as it matches the approach we've historically used for JVM features. New features are included under #ifdef making it:

• easy to find the feature-specific code
• easy to compile them in / out for performance and footprint comparisons
• easy to disable when issues are found.

Changes to improve & update common code won't require the ifdef. This results in the minimal set of feature-specific code and provides the greatest flexibility to determine the right default in the future.

I'd like to see the code merged into the master branch under #ifdef and "bake" there for a period before changing the default enablement for builds. Once the code is in, we can do the side-by-side comparisons and determine the right default for our builds both here and at Adopt. It's premature until then.

+1 to a configure / cmake option to make it easy to enable / disable building the JITaaS feature.

[mstoodle]

agree with @DanHeidinga : we do not have to choose to implement my B and C until we're sure that the feature is "baked", which is another advantage of approach 1.

but i think we should strive to get PR builds to include a JITaaS build as quickly as possible; otherwise we open ourselves to the "rot" problem.

[mpirvu]

Thanks for all your input. Taking all the feedback into account the current strategy leans towards a modified version of Approach 1 above:

1) Protect JITaaS code to #ifdef. Create a configure/cmake option to enable the ifdef and compile-in the JITaaS code
2) Let the JITaaS code "bake" for a while. Interim, write test cases that exercise corner cases, like network disconnects.
3) Change PR builds to build/test JITaaS code too. JITaaS code will be built only for Linux platforms and, to start with, only for Java8. This will avoid the "rot" problem while limiting the stress increase on the build/test machines.
4) Later on, change the build process at AdoptOpenJDK to include JITaaS in openj9 by default
5) Assuming everything goes well, automatically build JITaaS code in when dependencies (protobufs) are met. As @charliegracie suggested, autotools/CMake detection for JITaaS requirements should make life easier for developers at large. Add message to "java -Xinternalversion" to indicate support and also a line in javacore.

If anybody disagrees with this plan, please let us know.

[keithc-ca]

About the only change I would suggest is that the configure option should check for protobuf and any other requirements; until it becomes a feature by default, it should not be enabled just because the build machine is capable of having it enabled (following the principle of least surprise).

[mpirvu]

until it becomes a feature by default, it should not be enabled just because the build machine is capable of having it enabled (following the principle of least surprise).

Agreed