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

Implement PR testing for JITServer with the master branch #7782

Closed
mpirvu opened this issue Nov 18, 2019 · 6 comments · Fixed by #7860
Closed

Implement PR testing for JITServer with the master branch #7782

mpirvu opened this issue Nov 18, 2019 · 6 comments · Fixed by #7860
Labels
comp:build comp:jitserver Artifacts related to JIT-as-a-Service project

Comments

@mpirvu
Copy link
Contributor

mpirvu commented Nov 18, 2019

Currently we can do PR testing for the JITServer with the jittas branch with a comment like:
Jenkins test sanity.functional+jitaas xlinux jdk8,jdk11 depends eclipse/openj9-omr#jitaas
We would like to have the same functionality for the master branch as well, but in the master branch the JITServer code is protected by #ifdef JITSERVER_SUPPORT.
One possibility is to set the following env var: JITSERVER_SUPPORT=1 whenever +jitaas is seen in the comment that triggers the testing.
@AdamBrousseau proposed a better alternative: create new platform/spec on the compile side similar to how we do compressed vs XL. This is going to be a duplicate spec with an extra env variable.

@mpirvu mpirvu added comp:jitserver Artifacts related to JIT-as-a-Service project comp:build labels Nov 18, 2019
@mpirvu
Copy link
Contributor Author

mpirvu commented Nov 20, 2019

FYI: @pshipton @jdekonin

@pshipton
Copy link
Member

We should be using the —enable-jitserver configure option (ibmruntimes/openj9-openjdk-jdk8#346), and adding the support for that, similarly to how the cmake build works. If that is what create new platform/spec means, then I agree.

@AdamBrousseau
Copy link
Contributor

Yes. When I originally said new spec with extra env variable I didn't know about the work to add the new configure option. It would be a dup spec with one more config option. I would prefer to add this after #7809 is merged but I'm not sure if that will land in time for this work.

@AdamBrousseau
Copy link
Contributor

Few things to confirm.

  1. This will be similar to cmake jobs. Ie. separate compile jobs, same test jobs. Since test doesn't have reason to support dup specs, JITServer is passed to test via TEST_FLAG. This will mean mixed history on a test job between JITServer tests and regular tests.
  2. How does this affect the TEST_FLAG +jitaas. I'm wondering if I can remove this hackish check and explicitly set it based on the new spec.

ie. You would then be able to do

Jenkins test sanity.functional xlinux,xlinuxjit jdk8,11

Instead of

Jenkins test sanity.functional xlinux jdk8,11
Jenkins test sanity.functional+jitaas xlinuxjit jdk8,11

The side effect is you can only have jitserver testing on jitserver specs. Which I assume would be the case but please confirm.

Note to self, this mechanism is still used for +aot so it can't be completely removed but it can be removed for this case.

If we did want separate test jobs, we may need to pass a different IMPL (implementation) in order to get different test job names (jitserver?). Not sure if that makes sense or how much extra work that would be on the test side.
cc @smlambert

@smlambert
Copy link
Contributor

Do not believe there is any reason to having separate test job names, because these test jobs would be related to a parent pipelines that is jitserver specific (and so can be found & queried in jenkins & trss from parent jitserver build, fyi @llxia).

If you did want that, do not override JDK_IMPL or any of the core parameters that have a specific meaning and usage in underlying scripts (as it would have disastrous effects). If different name needed, recommendation would be to use a different SUFFIX a parameter that exists only to allow for different job names (currently _Nightly, _Release and _personal).

@mpirvu
Copy link
Contributor Author

mpirvu commented Nov 28, 2019

I don't have a preference for the options above.
Jenkins test sanity.functional xlinuxjit jdk8,11 is more compact while
Jenkins test sanity.functional+jitaas xlinuxjit jdk8,11 looks more like the AOT case.
Testing the xlinuxjit build in non-jitserver mode has little value now and in the future when jitserver code will be compiled-in by default will have no value.

AdamBrousseau added a commit to AdamBrousseau/openj9 that referenced this issue Dec 3, 2019
- Adds ability to compile with the JITServer flag (--enable-jitserver=yes).
- Adds ability to set TEST_FLAG for all test targets via variable file.
- Set TEST_FLAG=JITAAS via variable file instead of having to pass it with
  the test target.
- Still leave the option to set a TEST_FLAG via adding +FLAG to the test
  target name as this is used for +aot.
- Add JITServer specs for x/p/z Linux.

Fixes eclipse-openj9#7782
[skip ci]

Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:build comp:jitserver Artifacts related to JIT-as-a-Service project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants