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

More classloaders cleanup #40557

Merged
merged 2 commits into from
May 10, 2024
Merged

More classloaders cleanup #40557

merged 2 commits into from
May 10, 2024

Conversation

Sanne
Copy link
Member

@Sanne Sanne commented May 10, 2024

A very minor cleanup, but paving the road for other PRs which would otherwise depend on this.

  • Stop using JarClassPathElement.readStreamContents(InputStream) which has been deprecated for years
  • actually resolves a performance issue as this method is far less efficient in loading resources than the newly available alternatives
  • prepare IoUtil to be reused more by upcoming improvements in the Hibernate extension

I haven't deleted readStreamContents yet as there are some tests using it still.

@quarkus-bot quarkus-bot bot added area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels May 10, 2024
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I think this looks fine but I would like to let @dmlloyd have a look as he like low level stuff :).

@gsmet gsmet requested a review from dmlloyd May 10, 2024 15:14
Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

Looks good, but it is worth noting that for whatever reason, ZipFileInflaterInputStream does not optimize the array size used by readAllBytes, so there is going to be a buffered copy by that method in addition to the one potentially used internally by InflaterInputStream. Ideally we'd do the equivalent of readNBytes(entry.getUncompressedSize()) in this case. But, there are a lot of layers that make this difficult.

Copy link

quarkus-bot bot commented May 10, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5773913.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-6","-8","-9","-10","-11","-12","-13","-14"] to start with: ["-6", "-7", "-8", "-9"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-6","-8","-9","-10","-11","-12","-13","-14"]
to start with:
  ["-6", "-7", "-8", "-9"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:41)

@Sanne
Copy link
Member Author

Sanne commented May 10, 2024

@dmlloyd thanks for the pointer to ZipFileInflaterInputStream .. I'll have a look at how that might affect us, but not considering this a blocker for the current patch as the code I'm avoiding to use is always allocating buffers inefficiently.
So at least we'll have a better implementation sometimes :)

I noticed this profiling bootstraps cycles during live-reload; I had not noticed the inefficiency in ZipFileInflaterInputStream so this is useful to know, it's entirely possible the scenarious I was looking at didn't trigger it. Either way, this shouldn't be making it worse.

@Sanne Sanne merged commit 602e4c4 into quarkusio:main May 10, 2024
51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone May 10, 2024
@Sanne Sanne deleted the MoreClassloadersCleanup branch May 10, 2024 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants