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

Build perf app #254

Merged
merged 9 commits into from
Apr 9, 2024
Merged

Build perf app #254

merged 9 commits into from
Apr 9, 2024

Conversation

Karm
Copy link
Owner

@Karm Karm commented Apr 5, 2024

Adds a new build perf app that sports more dependencies, including two databases and tests for all demo functionality. Demo code is present so as the deps are not optimized away.

This PR is a WIP, I just confirmed it work with the oldest Q we work with in the TS regularly. I will add ports presently to this PR.

$ mvn clean verify -DexcludeTags=all -DincludeTags=perfcheck -Ptestsuite -Dtest=PerfCheckTest#testQuarkusMPOrmAwt -Dquarkus.version=2.13.3.Final -Dperf.app.report=true -Dperf.app.endpoint=http://localhost:8080 -Dperf.app.secret.token=manysecretcharacters


...
2024-04-05 12:13:06.875 INFO  [o.g.t.i.u.Uploader] (postBuildtimePayload) POSTing payload to http://localhost:8080/api/v1/image-stats/import?t=22.3.4-final,2.13.3.Final
2024-04-05 12:13:06.933 INFO  [o.g.t.i.u.Uploader] (postBuildtimePayload) POSTing payload to http://localhost:8080/api/v1/image-stats/194002
2024-04-05 12:13:06.947 INFO  [o.g.t.i.PerfCheckTest] (testQuarkusMPOrmAwt) Response code:200
2024-04-05 12:13:06.947 INFO  [o.g.t.i.PerfCheckTest] (testQuarkusMPOrmAwt) Response body:{"id":194002,"created_at":"2024-04-05T10:13:06.907+00:00","tag":"22.3.4-final,2.13.3.Final","img_name":"mp-orm-dbs-awt-runner","generator_version":"GraalVM 22.3.4.0-Final Java 17 Mandrel Distribution","image_size_stats":{"id":194002,"total_bytes":108991144,"code_cache_bytes":52835328,"heap_bytes":52580352,"resources_bytes":1471995,"resources_count":47,"other_bytes":3575464,"debuginfo_bytes":0},"jni_classes_stats":{"id":685005,"classes":178,"fields":1535,"methods":2092},"reflection_stats":{"id":685007,"classes":852,"fields":533,"methods":4464},"build_perf_stats":{"id":194002,"total_build_time_sec":114.003,"gc_time_sec":8.585,"num_cpu_cores":16,"total_machine_memory":66847313920,"peak_rss_bytes":6762926080,"cpu_load":9.896533342654653},"total_classes_stats":{"id":685008,"classes":27794,"fields":62023,"methods":216037},"reachability_stats":{"id":685006,"classes":25604,"fields":40448,"methods":126403}}

@Karm Karm self-assigned this Apr 5, 2024
@Karm Karm marked this pull request as draft April 5, 2024 10:27
@jerboaa
Copy link
Collaborator

jerboaa commented Apr 5, 2024

@Karm I'm a bit concerned that the test suite is growing too big and the burden to get them fixed becomes a problem. Could we perhaps ensure that those performance profiles don't run by default and only run when activated (which should be done on some jobs we run, but definitely not all). Especially in GHA, test failures pop up frequently with new warnings and produce noise (case in point: current test failures in this PR).

@Karm
Copy link
Owner Author

Karm commented Apr 5, 2024

Thx @jerboaa for the comment. FYI @zakkak

@Karm I'm a bit concerned that the test suite is growing too big and the burden to get them fixed becomes a problem. Could we perhaps ensure that those performance profiles don't run by default and only run when activated (which should be done on some jobs we run, but definitely not all). Especially in GHA, test failures pop up frequently with new warnings and produce noise (case in point: current test failures in this PR).

This PR is a draft for Quarkus 2.13.3 as the subject of the PR states, i.e. as you can see 2.13.3.Final GA jobs are green here.

Furthermore, this is affecting the perfcheck JUnit tag, and that is not executed at all by default:

   <includeTags>runtimes,reproducers</includeTags>

We enable it here on the CI because here we are "testing the testsuite".

Regarding adding apps:
As of this PR, apps/quarkus-full-microprofile is actually a subset of apps/quarkus-mp-orm-dbs-awt, so I think we can consider replacing the former with the latter. The one big change in that would be that the runtimes JUnit tag would suddenly require Podman/Docker so as Testcontainers could run and MariaDB and Postgres could be started.
It would be a new requirement, because the test suite does not require containers unless you specifically want container tests enabled by -Ptestsuite-builder-image. Given our environment, I think it's perhaps a safe thing to do to require containers on host by default.

EDIT: Meh. We can't replace one with the other. While apps/quarkus-full-microprofile natively builds on Windows, apps/quarkus-mp-orm-dbs-awt won't until we port Quarkus AWT extension to Windows. So disregard that. I could create submodules though.

@jerboaa
Copy link
Collaborator

jerboaa commented Apr 5, 2024

Furthermore, this is affecting the perfcheck JUnit tag, and that is not executed at all by default:

   <includeTags>runtimes,reproducers</includeTags>

We enable it here on the CI because here we are "testing the testsuite".

Mandrel CI uses this -Ptestsuite:
https://github.com/graalvm/mandrel/blob/d9566d884ad80014d430983e846ca01745950a50/.github/workflows/base.yml#L736-L738

So I'd assume it would run this one too?

@Karm
Copy link
Owner Author

Karm commented Apr 5, 2024

Furthermore, this is affecting the perfcheck JUnit tag, and that is not executed at all by default:

   <includeTags>runtimes,reproducers</includeTags>

We enable it here on the CI because here we are "testing the testsuite".

Mandrel CI uses this -Ptestsuite: https://github.com/graalvm/mandrel/blob/d9566d884ad80014d430983e846ca01745950a50/.github/workflows/base.yml#L736-L738

So I'd assume it would run this one too?

Nope, see it's tagged perfcheck here.
And that tag is not included in the profile here.

@jerboaa
Copy link
Collaborator

jerboaa commented Apr 5, 2024

OK. Thanks. I'm probably confusing it with the JFR perf test then.

@Karm
Copy link
Owner Author

Karm commented Apr 5, 2024

OK. Thanks. I'm probably confusing it with the JFR perf test then.

That is right, we wanted those on even for regular runs, so they are annotated as reproducers here.

The fact that it confuses you means I botched the user experience of the tags taxonomy. I guess we can refactor that to some more logical parts.

@Karm Karm marked this pull request as ready for review April 8, 2024 11:30
@Karm Karm changed the title [WIP] Build perf app, Q 2.13.3.Final Build perf app Apr 8, 2024
@Karm Karm requested a review from jerboaa April 8, 2024 12:25
@Karm
Copy link
Owner Author

Karm commented Apr 8, 2024

  • I'll try with today's Q main now locally.
  • And check on aarch64

testsuite/pom.xml Outdated Show resolved Hide resolved
Comment on lines +152 to +154
// RestEasy intermittently
p.add(Pattern.compile(".*Closing a class org.jboss.resteasy.client.*"));
p.add(Pattern.compile(".*Please close clients yourself.*"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this is a quarkus problem or a test issue that needs handling there. Either way, further investigation seems waranted. See quarkusio/quarkus#10813 (comment)

Copy link
Owner Author

Choose a reason for hiding this comment

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

I thought it was me, not bothering with the rest client here:

https://github.com/Karm/mandrel-integration-tests/blob/master/apps/quarkus-full-microprofile/src/main/java/com/example/quarkus/secure/TestSecureController.java#L45

So I did treat the closable this time around:

https://github.com/Karm/mandrel-integration-tests/pull/254/files#diff-e855f86a0889eaeb6ad4c059c62b3d11064d2a7e2d57a44d6df6d70e6c9507f9R49

...to no avail. IMHO it's something else. Like CDI scope being wrong? Perhaps it's expected to be just RequestScoped and by minimizing boilerplate and jamming everything together in App scoped in the test case, we are triggering it. It's not specific to native though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm happy enough to have a quarkus issue created which tracks investigating it, fwiw.

Copy link
Collaborator

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

OK, given that you'll file a quarkus issue on the http components client closing issue. Thanks!

@Karm
Copy link
Owner Author

Karm commented Apr 9, 2024

OK, given that you'll file a quarkus issue on the http components client closing issue. Thanks!

THX for the review. I've bee cooking this one for some days :-)

Ad issue: I'll give it an hour to explore the CDI scope and fixing it in the app code, if it fails, I'll open an issue with a reproducer, linking to the old one.

@Karm Karm merged commit abe8807 into master Apr 9, 2024
50 checks passed
@Karm
Copy link
Owner Author

Karm commented Apr 11, 2024

OK, given that you'll file a quarkus issue on the http components client closing issue. Thanks!

@jerboaa I am working on that in a separate PR. I found out that the problem disappears if wrap in try-with-resources...except for some older versions, where either as a bug or as a feature, the Client isn't Closable and thus you have to call .close() manually in finally block the old way. I am testing it now, but it seems it's just a clumsy API inconsistencies over the rest easy versions in Quarkus that could be dealt with in the user's codebase.

@jerboaa
Copy link
Collaborator

jerboaa commented Apr 11, 2024

I found out that the problem disappears if wrap in try-with-resources

That's what I thought. So we ought to fix the test and then remove the allow-listing.

@Karm
Copy link
Owner Author

Karm commented Apr 11, 2024

I found out that the problem disappears if wrap in try-with-resources

That's what I thought. So we ought to fix the test and then remove the allow-listing.

It is happening in #255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants