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
Retrieve constant dynamic info from the JITClient #8277
Conversation
85803e3
to
bc89bcd
Compare
83041a5
to
ec633b3
Compare
|
Looking at how this is used: That deference, |
|
In general, every time the mainline acquires VM access, the JITServer implementation may have a problem. I am seeing the same pattern in another place: |
|
The dereference is taken care of by this PR. The pointer will be dereferenced on the client side and sent back to the server in To consolidate the messages sent between the server and the client, the existing API [1] |
ec633b3
to
7dc03c1
Compare
|
@cathyzhyi Could you help review the above suggestion on changing the |
The proposed change sounds good to me. IMO, it’s better to keep the original API that only takes Some comments for the new API saying the extra argument is for jit as a service is also needed. |
7dc03c1
to
1c2a03a
Compare
|
eclipse/omr#4726 is created to add the new API. On a second thought, I think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some comments
872b366
to
4e10bcd
Compare
4e10bcd
to
62cb7de
Compare
|
Another issue I forgot about: if there is a chance that these changes may break the compatibility with an older server/client, then we have to increment the version number. |
f38a319
to
4dab423
Compare
4dab423
to
b135870
Compare
|
@mpirvu All comments addressed. Ready for another review. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Jenkins test sanity xlinuxjit jdk11 |
|
Xlinux JDK11 test passed on non-JITServer as below: |
|
@a7ehuo Could you please rebase this PR? There is an openj9 PR that I just merged which will enable again JITServer testing. Thanks |
b135870
to
44d7d1f
Compare
|
@mpirvu Rebased the PR into the commit 44d7d1f. |
|
Jenkins test sanity xlinuxjit jdk11 |
|
|
|
This PR fixes the JITServer crash when running sanity test |
|
Thanks. Will wait for testing on 8370. |
Dynamic constant is not stored at the JITServer. The server will send one message to retrieve both the pointer and the object values in `TR_ResolvedJ9JITServerMethod::dynamicConstant()`. It has dependency on the new `dynamicConstant` API defined in eclipse/omr#4726. Also increased the JITServer `MINOR_NUMBER` to `3` since the new message types have to be handled at the client as well. This commit fixes the server crash when running Java 11 sanity test `CondyPrimitive_1`. Related to eclipse-openj9#7567 Signed-off-by: Annabelle Huo <Annabelle.Huo@ibm.com>
44d7d1f
to
5b525b7
Compare
|
Jenkins test sanity xlinuxjit,plinuxjit jdk11 |
|
|
Reproduced the same crash with the |
|
Yeah, I see |
|
Since the errors seen in testing are not due to this PR, I am going to merge it. |
Dynamic constant is not stored at the
JITServer. It needs to be retrieved from the client side. This PR has dependency on the newdynamicConstantAPI defined in eclipse/omr#4726.Also increased the JITServer
MINOR_NUMBERto3since the new message types have to be handled at the client as well.This commit fixes the server crash when running
Java 11sanity testCondyPrimitive_1.Related to #7567.
Signed-off-by: Annabelle Huo Annabelle.Huo@ibm.com