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

Add a simple cache for ComparableVersions #870

Merged
merged 1 commit into from Dec 21, 2022
Merged

Add a simple cache for ComparableVersions #870

merged 1 commit into from Dec 21, 2022

Conversation

TobiX
Copy link
Contributor

@TobiX TobiX commented Dec 20, 2022

This is a simple fix for #869.

This can improve performance in pathetic corner cases by up to 50%. From some example executions (something similar as the pathetic case in #869):

Before:

[INFO] Total time:  01:51 min

After:

[INFO] Total time:  43.714 s

In this first implementation, the cache is unbounded. In all my tests it never grew over ~1500 entries, but it might keep some memory active in pathetic cases...

PS: I used a ConcurrentHashMap, because I don't know how parallel (if at all?) Maven can run this code...

This can improve performance in pathetic corner cases by up to 50%.
@slawekjaranowski slawekjaranowski linked an issue Dec 21, 2022 that may be closed by this pull request
@slawekjaranowski slawekjaranowski added this to the 2.14.2 milestone Dec 21, 2022
@slawekjaranowski slawekjaranowski merged commit 2bed457 into mojohaus:master Dec 21, 2022
@TobiX TobiX deleted the cache-comparableversion branch December 21, 2022 17:36
@jarmoniuk
Copy link
Contributor

I was looking into moving this to Maven level. However:

Neither ComparableVersion nor DefaultArtifactVersion are immutable: parseVersion will mutate their state. If this is the case, they mustn't be cached.

In the plugin, we must therefore forbid ComparableVersion users from ever using parseVersion() on the instance retrieved from cache.

@TobiX
Copy link
Contributor Author

TobiX commented Jan 4, 2023

@ajarmoniuk Oh right, parseVersion is public. That's unfortunate (aka. a bug waiting to happen 😉)... I found no local users of that method, but it might be considered API-breaking to make it private... Would an "immutable" subclass be a viable solution?

@jarmoniuk
Copy link
Contributor

@TobiX well, that's indeed unfortunate. I wanted to promote your cache to maven-core, but looked into it closer and discovered that it's in fact mutable (because of that method). The interface is a bit... botched and it would be really nice to change it, but - Maven is big and who knows who depends on it.

What's more -- I saw that we could actually cache DefaultArtifactVersion instances, since they are also immutable except for that unfortunate method, which is also part of the interface. And we're actually constructing a lot more of those objects, and use them for comparison.

So, instead, I've introuced this change: #898

we're caching DefaultArtifactVersions instead and only in the plugin -- on condition that we will never call parseVersion in our plugin.

@jarmoniuk
Copy link
Contributor

dongjoon-hyun pushed a commit to apache/spark that referenced this pull request Jan 29, 2023
### What changes were proposed in this pull request?
This pr aims upgrade `versions-maven-plugin` to 2.14.2

### Why are the changes needed?
New version bring some improvement like [Add a simple cache for ComparableVersions
](mojohaus/versions#870)
The full release notes as follows:
- https://github.com/mojohaus/versions/releases/tag/2.14.2

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?

- GA `Dependencies test` should work normally
- Manually check `./dev/test-dependencies.sh --replace-manifest`, run successful

Closes #39784 from LuciferYang/SPARK-42226.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
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.

Performance issue with many versions/artifacts
3 participants