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

Avoid building a URI again and again in JarResource #39508

Closed
wants to merge 1 commit into from

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Mar 16, 2024

@franz1981 could you have a look at this change? It's related to #39447 .

@quarkus-bot quarkus-bot bot added the area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins label Mar 16, 2024
Copy link

quarkus-bot bot commented Mar 16, 2024

Status for workflow Quarkus CI

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

✅ 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.

@franz1981
Copy link
Contributor

now checking @gsmet!

@franz1981
Copy link
Contributor

adding @geoand too which is the one who wrote this one

I'm mostly checking there's some evident performance reason why it shouldn't be the same (which doesn't seem the case , at a first look)

@gsmet
Copy link
Member Author

gsmet commented Mar 18, 2024

Shouldn't be the same as what?

@franz1981
Copy link
Contributor

franz1981 commented Mar 18, 2024

I mean, it should be the same as it was before (from a semantic perspective), but i'm double-checking if there is any performance unexpected reason to not be the same (or better)

@franz1981
Copy link
Contributor

franz1981 commented Mar 18, 2024

So, in theory JarResource::init happens just once at

, meaning that there's not reason to cache URI jarUri as instance field and can just chain jarPath.toUti().toURL() on it, to save retaining more heap and necessary and let URI to be Gc'd.
I'm now checking if the allocated bytes are the same for the 2 methods.

@gsmet
Copy link
Member Author

gsmet commented Mar 18, 2024

Have you looked at the call I removed? Because that was the whole purpose of this patch. Removing a call that we do a lot of times for the config stuff.

@franz1981
Copy link
Contributor

@franz1981
Copy link
Contributor

franz1981 commented Mar 18, 2024

TLDR for the getting-started quickstart I cannot see any measurable difference, because it seems getResourceURL is called just 9 times (i've attached the profiling data both for allocation(s) and number of times the method is called and where)
profling_data.zip

The reason why the concat indy was visible, is that just a single call can trigger bytecode generation there, and that's why it was worthy to be fixed

Said that I'm all in to simplify the existing code path anyway, unless @geoand got some reason why it was like that; which I'm not very familiar with

@gsmet
Copy link
Member Author

gsmet commented Mar 18, 2024

OK, well, if it doesn't bring anything to the plate, we can close this.

@gsmet gsmet closed this Mar 18, 2024
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins triage/invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants