-
Notifications
You must be signed in to change notification settings - Fork 4
[WIP] Started implementing compareNativeAndJVMSerialGCTime test #355
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
Conversation
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.
A few initial comments.
testsuite/src/it/java/org/graalvm/tests/integration/utils/BuildAndRunCmds.java
Outdated
Show resolved
Hide resolved
testsuite/src/it/java/org/graalvm/tests/integration/PerfCheckTest.java
Outdated
Show resolved
Hide resolved
FYI: Took some break on this, since I was busy with other stuff and had a PTO, I will continue with the work here now. |
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.
Thx Patrik,
When you address Severin's and mine concerns, either by doing what we suggest or by explaining why you would do it differently, could you please:
- Take a look at the API call Hyperfoil is making to the Quarkus app? Perhaps a wrong port? URL?
Error: PerfCheckTest.compareNativeAndJVMSerialGCTime:628 build-and-run.log log should not contain error or warning lines that are not whitelisted. See /home/runner/work/mandrel-integration-tests/mandrel-integration-tests/ts/testsuite/target/archived-logs/org.graalvm.tests.integration.PerfCheckTest/compareNativeAndJVMSerialGCTime/build-and-run.log and check these offending 399 lines:
15:21:14,137 WARN (epollEventLoopGroup-2-1) [i.h.h.h.RangeStatusValidator] #11 Sequence test(0)(4/4), request (#11, RUNNING, INVALID) GET localhost:8080/data/client/test//bonjour on connection Http1xConnection{/127.0.0.1:59476 -> localhost/127.0.0.1:8080, status=OPEN, size=1+0:[(#11, RUNNING, INVALID) GET localhost:8080/data/client/test//bonjour]} received invalid status 404
15:21:16,096 WARN (epollEventLoopGroup-2-1) [i.h.h.h.RangeStatusValidator] #95 Sequence test(0)(4/4), request (#95, RUNNING, INVALID) GET localhost:8080/data/client/test//helloThere on connection Http1xConnection{/127.0.0.1:60140 -> localhost/127.0.0.1:8080, status=OPEN, size=1+0:[(#95, RUNNING, INVALID) GET localhost:8080/data/client/test//helloThere]} received invalid status 404
15:21:17,041 WARN (epollEventLoopGroup-2-1) [i.h.h.h.RangeStatusValidator] #84 Sequence test(0)(4/4), request (#84, RUNNING, INVALID) GET localhost:8080/data/client/test//bonjour on connection Http1xConnection{/127.0.0.1:60562 -> localhost/127.0.0.1:8080, status=OPEN, size=1+0:[(#84, RUNNING, INVALID) GET localhost:8080/data/client/test//bonjour]} received invalid status 404
- Could you analyze the output and form some basic thresholds akin to what JFR tests do, e.g.
** native run should not perform worse than n % on the hyperfoil percentiles than the HotSpot run
** time spent in GC should not be worse in native than n % of the HotSpot
** or think of another strategy what would track and safeguard native perf smoke test compared to HotSpot
scenario: | ||
- test: | ||
# Step `randomItem` randomly picks one item from the list below... | ||
- randomItem: |
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.
@patrikcerbak I don't think a random pick here is a good idea. Is there a deterministic, sequential alternative?
The thing is that these APIs touch vastly different libraries and if one fails we kinda want to know it in a predictable order.
Speak up if you think it's not important though...
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.
Okay, that makes sense. I'll try to replace it by a deterministic alternative.
testsuite/src/it/java/org/graalvm/tests/integration/PerfCheckTest.java
Outdated
Show resolved
Hide resolved
testsuite/src/it/java/org/graalvm/tests/integration/PerfCheckTest.java
Outdated
Show resolved
Hide resolved
"Main is still open."); | ||
Logs.checkLog(cn, mn, app, processLog); | ||
|
||
// TODO, need to implement some asserting here once the timeSpentInGCs value is not 0 |
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 also think a sanity check like that would be nice here.
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.
Will do!
testsuite/src/it/java/org/graalvm/tests/integration/utils/BuildAndRunCmds.java
Outdated
Show resolved
Hide resolved
@Karm, I think I fixed the issue, there was a double slash character in the endpoints. Can you please approve the workflows here, so I can be check and be sure it's fixed? |
@patrikcerbak I invited you as a Collaborator on this project so as you can run CI as you please. |
Thanks! |
d542761
to
0701c42
Compare
Hi @Karm, I did some changes in the Commands.java file here, that should fix that: here. Would you mind looking at the few lines I added, when you have time? I want to be sure I used the PS: Sorry for the force push, I created some git problems locally... |
@patrikcerbak Force push is fine in these woods, as long as it's in your own branch :-) |
This is a new, non-whitelisted error:
Could you poke around, check Quarkus github, Netty and vertx githubs etc. and form an idea whether it's a benign thing or some kind of race we should pursue, please? |
Definitely, I'll investigate the error! What is weird is that it seems to appear very randomly (even when I'm running the test locally). |
@patrikcerbak Any news on this? Is it a safe candidate for WhiteList? |
@Karm I'm inclined to say yes. The only meaningful conversation I found about this is here: https://groups.google.com/g/vertx/c/ekzl1sagkVU. Other than that, I mostly found some unresolved issues. I also tried experimenting with changing the Hyperfoil config file and it seemed like that in some variants, the problem is hit less frequently. At this moment, I have a benchmark running locally to see if it is really the case or just luck (but it may take few more hours to run)... |
Well, it seems that it was just luck. In my opinion, I would say it is okay to add it to the whitelist. |
@patrikcerbak Ack. Whitelist with a comment, rebase, CI... Thx. 🙏 |
…ng the GC performance in native and JVM
0701c42
to
693ece6
Compare
|
Yes, that is a new (sanity) assert that I added. Seems like the issue is that it wrongly analyzes the logs for GC pause times, again. I'll investigate -- hopefully it's an easy fix. |
Mandrel 24.x seems to put an extra |
@patrikcerbak I think It's fine now. Didn't you find the random pattern in the Hyperfoil config problematic? |
Yes, it certainly is problematic, I agree with that. However, when I did some digging into the Hyperfoil configs, i didn't find an easy "linear" replacement for it, all of the options seemed overly complicated. But I'll try looking into it a bit more and I'll let you know. |
…not choose a random one
In the last commit, I changed it, so that Hyperfoil makes requests all of the URLs one by one, and does not pick a random one. (And also tweaked some parameters in the config for it to work.) Maybe I'm missing something, but I found that to be the sanest way to do it. What do you think? Should I leave it like this? If so (and everything finishes okay), I'm changing this from a draft to a PR. |
@patrikcerbak Ack. Thx. Let's wait for the CI. 🙏 |
It will probably need some more changes to the config parameters, I'm working on it now... |
057e034
to
0eee14e
Compare
Hi, sorry for the delay, I was busy with other stuff. However, I did a lot of testing locally and I must say that I didn't find any Hyperfoil configuration, which would have a 100% success rate. It still, unfortunately, sometimes prints the However, since it only is a warning (and I don't think that throttling the benchmark execution will really mess with the GC times), I added the warning to the whitelist in the last commit. If you think that it is not the right way, let me know, I will revert the commit and try to find some other solution (even thought I don't think there is an easy one for this). |
Ack. Thank you very much. Rebase and let's merge. |
Covers the following 4 tests: - JFRTest#jfrPerfTest - DebugSymbolsTest#debugSymbolsQuarkus - RuntimesSmokeTest#helidonQuickStart - RuntimesSmokeTest#quarkusFullMicroProfile Closes: Karm#364
2e5c399
to
9c3b09e
Compare
Rebased, sorry for the mess. Will you pull the lever? |
@patrikcerbak Did you do a rebase or something else? It seems to me I can see Severin's commit in this set... |
@Karm |
Do you think it is ready for the merge or should I try to clean up locally and then force push? :) |
Thx , merging. |
Work in progress
Hi,
I started to implement
compareNativeAndJVMSerialGCTime
for comparing native vs JVM SerialGC times and performance.You can run it like so:
mvn clean verify -DincludeTags=perfcheck -Dtest=PerfCheckTest#compareNativeAndJVMSerialGCTime -Ptestsuite -Dquarkus.version=3.23.0 -Dquarkus.native.container-runtime=podman