8378257: [IR Framework] Add identity for socket connection and prepare for parsing C2 dumps#30536
8378257: [IR Framework] Add identity for socket connection and prepare for parsing C2 dumps#30536chhagedorn wants to merge 2 commits intoopenjdk:masterfrom
Conversation
…e for parsing C2 dumps
|
👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@chhagedorn The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
mhaessig
left a comment
mhaessig
left a comment
There was a problem hiding this comment.
Thank you for the continuous improvement of the IR framework, @chhagedorn! I have a question and a nit.
test/hotspot/jtreg/compiler/lib/ir_framework/shared/TestFrameworkSocket.java
Outdated
Show resolved
Hide resolved
| try { | ||
| client.setSoTimeout(10000); | ||
| identity = reader.readLine(); | ||
| client.setSoTimeout(0); |
There was a problem hiding this comment.
Is 0 really the default or indeed a sensible timeout?
There was a problem hiding this comment.
0 corresponds to no timeout/infinite which is the default. I only temporarily set it to 10s to catch issues early when no connection can be established initially.
There was a problem hiding this comment.
In that case it makes sense :)
There was a problem hiding this comment.
It probably doesn't make any difference in practice, but should the timeout reset be put in a finally block to be sure that it is always reset?
…orkSocket.java Co-authored-by: Manuel Hässig <manuel@haessig.org>
|
Thanks for your review @mhaessig! I've pushed an update fixing the typos. |
mhaessig
left a comment
mhaessig
left a comment
There was a problem hiding this comment.
Thank you for addressing my comment and answering my question. Looks good.
dafedafe
left a comment
dafedafe
left a comment
There was a problem hiding this comment.
Thank you @chhagedorn! Looks good to me. I just have a couple of small things/questions...
| Socket client = serverSocket.accept(); | ||
| BufferedReader reader = new BufferedReader(new InputStreamReader(client.getInputStream())); | ||
| submitTask(client, reader); | ||
| String identity = readIdentity(client, reader).trim(); |
There was a problem hiding this comment.
Could readIdentity return null? If so, should that be specifically handled, or is it OK to pass it up? (It will throw a TestFrameworkException in the caller but maybe we want a different message...)
| try { | ||
| client.setSoTimeout(10000); | ||
| identity = reader.readLine(); | ||
| client.setSoTimeout(0); |
There was a problem hiding this comment.
It probably doesn't make any difference in practice, but should the timeout reset be put in a finally block to be sure that it is always reset?
3 participants
This patch adds an identity handshake for the Test VM client connection to the
TestFrameworkSocket. The identity will be required once we also send dumps from C2 to differentiate between different connections.As an additional preparation step, I already split off the shared
TestVmMessageParserinterface from the full patch that is later also implemented by the C2 method dump parser (no semantic change).Thanks,
Christian
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30536/head:pull/30536$ git checkout pull/30536Update a local copy of the PR:
$ git checkout pull/30536$ git pull https://git.openjdk.org/jdk.git pull/30536/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30536View PR using the GUI difftool:
$ git pr show -t 30536Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30536.diff
Using Webrev
Link to Webrev Comment