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

Collect stats about sizes of different types of messages sent in JITServer #9752

Merged
merged 1 commit into from
Jun 6, 2020

Conversation

EmanElsaban
Copy link
Contributor

I'm keeping track of the size of different types of messages sent using msg.type()
and I'm using the class TR_Stats to store avg,min,max and stddev of every type of message.
issue: #9708

Signed-off-by: Eman Elsabban eman.elsaban1@gmail.com

@EmanElsaban EmanElsaban changed the title Collect stats about sizes of different types of messages sent Collect stats about sizes of different types of messages sent in JITServer May 31, 2020
@EmanElsaban EmanElsaban marked this pull request as draft May 31, 2020 20:17
@EmanElsaban EmanElsaban marked this pull request as ready for review June 1, 2020 12:30
@EmanElsaban
Copy link
Contributor Author

@mpirvu Can you please review my PR? Thank you.

@mpirvu mpirvu added the comp:jitserver Artifacts related to JIT-as-a-Service project label Jun 1, 2020
@mpirvu mpirvu added this to In progress in JIT as a Service via automation Jun 1, 2020
@mpirvu mpirvu self-assigned this Jun 1, 2020
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my inline comments.

runtime/compiler/control/CompilationRuntime.hpp Outdated Show resolved Hide resolved
runtime/compiler/control/CompilationRuntime.hpp Outdated Show resolved Hide resolved
runtime/compiler/control/JITServerHelpers.cpp Outdated Show resolved Hide resolved
runtime/compiler/net/CommunicationStream.cpp Outdated Show resolved Hide resolved
runtime/compiler/net/CommunicationStream.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks good. Most of the time we would not need this code, so to avoid the extra overhead I would prefer to have it protected by an inactive #ifdef.

@EmanElsaban EmanElsaban force-pushed the collectStats branch 2 times, most recently from 25e3599 to bc03e03 Compare June 5, 2020 14:14
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more request: looking at other usages, ifdefs start in column 0. Also, there is another define called STATS in other part of the code. To be more specific we should call this something like `MESSAGE_SIZE_STATS

I'm keeping track of the size of different types of messages sent using msg.type()
and I'm using the class TR_Stats to store avg,min,max and stddev of every type of message.
I declared a static array in CommunicationStream.hpp and I'm using that to store in.
issue: eclipse-openj9#9708

Signed-off-by: Eman Elsabban <eman.elsaban1@gmail.com>
Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mpirvu
Copy link
Contributor

mpirvu commented Jun 5, 2020

jenkins test sanity plinuxjit jdk11

@mpirvu
Copy link
Contributor

mpirvu commented Jun 5, 2020

@EmanElsaban Would be able to run this code through some benchmark (AcmeAir would be a good choice) and collect the statistics?

@mpirvu mpirvu merged commit 02b97c2 into eclipse-openj9:master Jun 6, 2020
JIT as a Service automation moved this from In progress to Done Jun 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:jitserver Artifacts related to JIT-as-a-Service project
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants