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

Cache result of getBestFitRule (fixes #575) #647

Merged
merged 1 commit into from Aug 25, 2022
Merged

Cache result of getBestFitRule (fixes #575) #647

merged 1 commit into from Aug 25, 2022

Conversation

TobiX
Copy link
Contributor

@TobiX TobiX commented Aug 23, 2022

This was identified as a performance hotspot and the result is static
for each run, adding a cache dramatically speeds up some executions.

The included IT shows the problem: Without this optimization, it runs
over a minute, with the fix it finishes instantly.

(Reopened from #365 because I did stupid things with our fork)

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

If we have another IT test which execute: property-updates-report

I would consider to remove old, after add moving assertions to this test.
One test will be enough.

src/it/it-property-updates-report-002-slow/pom.xml Outdated Show resolved Hide resolved
src/it/it-property-updates-report-002-slow/pom.xml Outdated Show resolved Hide resolved
@slawekjaranowski
Copy link
Member

There is: https://github.com/mojohaus/versions-maven-plugin/tree/master/src/it/it-property-updates-report-001
can be merged to one test.

Pleas check build status 😄

@TobiX
Copy link
Contributor Author

TobiX commented Aug 23, 2022

Hmm, I'm used to write one test per issue, but I can certainly extend the existing test to also show this problem...

@slawekjaranowski
Copy link
Member

IT tests take long time - I try to minimize duplicate test.
If you think that both tests do something else - ok, we can have its.

@TobiX
Copy link
Contributor Author

TobiX commented Aug 23, 2022

I tried to combine both ITs, unfortunately the validation script for the existing test is quite sensitive to dependency changes, so for the sake of stability I'd rather not combine both tests...

@slawekjaranowski
Copy link
Member

I see a warning in IT build ...

[INFO] Building: it-property-updates-report-002-slow/pom.xml
[INFO] artifact org.junit.jupiter:junit-jupiter: checking for updates from central
[INFO] artifact org.junit.jupiter:junit-jupiter-engine: checking for updates from central
Warning:  Could not validate integrity of download from https://repo.maven.apache.org/maven2/org/junit/jupiter/junit-jupiter-engine/maven-metadata.xml.sha1
org.eclipse.aether.transfer.ChecksumFailureException: Checksum validation failed, no checksums available
...
[INFO] artifact org.junit.jupiter.junit-jupiter-engine:junit-jupiter-engine: checking for updates from central
Warning:  /org/junit/jupiter/junit-jupiter-engine/maven-metadata.xml.sha1
java.lang.NullPointerException
    at org.codehaus.mojo.mrm.impl.digest.SHA1DigestFileEntry.<init> (SHA1DigestFileEntry.java:67)
    at org.codehaus.mojo.mrm.impl.digest.SHA1DigestFileEntry$Factory.create (SHA1DigestFileEntry.java:148)

maybe it is a bug in mrm plugin ... but I would not introduce such warning

can you a litel investigate it?

This was identified as a performance hotspot and the result is static
for each run, adding a cache dramatically speeds up some executions.

The included IT shows the problem: Without this optimization, it runs
over a minute, with the fix it finishes instantly.
@TobiX
Copy link
Contributor Author

TobiX commented Aug 24, 2022

I just dropped that particular dependency from the test... It's still plenty slow before my change to fail the test before my fix...

@slawekjaranowski slawekjaranowski merged commit 6c6688b into mojohaus:master Aug 25, 2022
@slawekjaranowski
Copy link
Member

@TobiX - Thanks

@TobiX TobiX deleted the cache-expensive-calls branch August 25, 2022 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

property-updates-report is slow with many dependencies & rules
2 participants