You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2023/02/08 15:06:20 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request, #241: [MRESOLVER-320] Use strong references
cstamas opened a new pull request, #241:
URL: https://github.com/apache/maven-resolver/pull/241
This increases memory usage (500MB vs 1.1G on tested project), but this is on par with Maven 3.8.7 behaviour and ceases the slowdown as well (speed goes back on par as well).
---
https://issues.apache.org/jira/browse/MRESOLVER-320
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #241: [MRESOLVER-320] Use strong references by default
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1105963478
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -80,14 +76,14 @@ public DataPool( RepositorySystemSession session )
if ( cache != null )
{
- artifacts = (ObjectPool<Artifact>) cache.get( session, ARTIFACT_POOL );
- dependencies = (ObjectPool<Dependency>) cache.get( session, DEPENDENCY_POOL );
- descriptors = (Map<Object, WeakReference<Descriptor>>) cache.get( session, DESCRIPTORS );
+ artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.get( session, ARTIFACT_POOL );
+ dependencies = (ConcurrentHashMap<Dependency, Dependency>) cache.get( session, DEPENDENCY_POOL );
+ descriptors = (ConcurrentHashMap<Object, Descriptor>) cache.get( session, DESCRIPTORS );
Review Comment:
True, but even if this method present (on cache), cache itself is nullable, so we cannot just drop all this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #241: [MRESOLVER-320] Use strong references
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1100334056
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -80,14 +76,14 @@ public DataPool( RepositorySystemSession session )
if ( cache != null )
{
- artifacts = (ObjectPool<Artifact>) cache.get( session, ARTIFACT_POOL );
- dependencies = (ObjectPool<Dependency>) cache.get( session, DEPENDENCY_POOL );
- descriptors = (Map<Object, WeakReference<Descriptor>>) cache.get( session, DESCRIPTORS );
+ artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.get( session, ARTIFACT_POOL );
+ dependencies = (ConcurrentHashMap<Dependency, Dependency>) cache.get( session, DEPENDENCY_POOL );
+ descriptors = (ConcurrentHashMap<Object, Descriptor>) cache.get( session, DESCRIPTORS );
Review Comment:
maybe, but according to japicmp that above is incompatible change.... and we promise this about API https://maven.apache.org/resolver/api-compatibility.html
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] gnodet commented on pull request #241: [MRESOLVER-320] Use strong references
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#issuecomment-1422822642
Also, wouldn't it be preferable to intern the artifacts and dependencies earlier, i.e. at creation time, rather than later ? Don't most created artifacts / dependencies end up in the end result ?
And subsequent question, have you tried without pooling at all ? To see the difference in perfs / memory consumption ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #241: [MRESOLVER-320] Use strong references by default
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1106003248
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -53,6 +52,7 @@
*/
public final class DataPool
{
+ private static final String CONFIG_PROP_COLLECTOR_POOL_WEAK = "aether.dependencyCollector.pool.weak";
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #241: [MRESOLVER-320] Use strong references
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1100517830
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -80,14 +76,14 @@ public DataPool( RepositorySystemSession session )
if ( cache != null )
{
- artifacts = (ObjectPool<Artifact>) cache.get( session, ARTIFACT_POOL );
- dependencies = (ObjectPool<Dependency>) cache.get( session, DEPENDENCY_POOL );
- descriptors = (Map<Object, WeakReference<Descriptor>>) cache.get( session, DESCRIPTORS );
+ artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.get( session, ARTIFACT_POOL );
+ dependencies = (ConcurrentHashMap<Dependency, Dependency>) cache.get( session, DEPENDENCY_POOL );
+ descriptors = (ConcurrentHashMap<Object, Descriptor>) cache.get( session, DESCRIPTORS );
Review Comment:
As of Chapter 13. of the [JLS](https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.5.3):
> Adding a default method, or changing a method from abstract to default, does not break compatibility with pre-existing binaries, but may cause an IncompatibleClassChangeError if a pre-existing binary attempts to invoke the method.
Such usage is unexpected to say the least, so japicmp goes on the safe side, but that should not be a problem really. See https://github.com/siom79/japicmp/issues/201
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #241: [MRESOLVER-320] Use strong references by default
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1105977003
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -415,4 +440,46 @@ public int hashCode()
return hashCode;
}
}
+
+ private interface InternPool<K, V>
+ {
+ V get( K key );
+
+ V intern( K key, V value );
+ }
+
+ private static class HardInternPool<K, V> implements InternPool<K, V>
+ {
+ private final ConcurrentHashMap<K, V> map = new ConcurrentHashMap<>( 256 );
+
+ @Override
+ public V get( K key )
+ {
+ return map.get( key );
+ }
+
+ @Override
+ public V intern( K key, V value )
+ {
+ return map.computeIfAbsent( key, k -> value );
+ }
+ }
+
+ private static class WeakInternPool<K, V> implements InternPool<K, V>
+ {
+ private final WeakHashMap<K, WeakReference<V>> map = new WeakHashMap<>( 256 );
+
+ @Override
+ public V get( K key )
Review Comment:
I think it's missing a `synchronized` flag to support concurrent read/write access, as I don't think the `WeakHashMap` does support that.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -53,6 +52,7 @@
*/
public final class DataPool
{
+ private static final String CONFIG_PROP_COLLECTOR_POOL_WEAK = "aether.dependencyCollector.pool.weak";
Review Comment:
Maybe use a non boolean property `aether.dependencyCollector.pool` so that we can experiment with a `weak`, `hard`, `none` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #241: [MRESOLVER-320] Use strong references
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#issuecomment-1429907311
Reworked a bit: I simply hated the fact that this PR would potentially make Resolver unable to calculate (some probably bigger graph) of dependencies in case of confined memory, even at cost of 10% speed reduction.
Hence, reworked the PR a bit: added a config, that by default makes resolver behave as with original patch (aggressive heap use, loose the 10% slow down), but, in case the behavior can be returned the 1.8.x resolver way (using weak pools).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on pull request #241: [MRESOLVER-320] Use strong references by default
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#issuecomment-1429962797
All good ideas! now we support "hard" (default), "weak" (the 1.8 way) and "none" (for experiment sake) pools
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #241: [MRESOLVER-320] Use strong references
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1100314432
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -80,14 +76,14 @@ public DataPool( RepositorySystemSession session )
if ( cache != null )
{
- artifacts = (ObjectPool<Artifact>) cache.get( session, ARTIFACT_POOL );
- dependencies = (ObjectPool<Dependency>) cache.get( session, DEPENDENCY_POOL );
- descriptors = (Map<Object, WeakReference<Descriptor>>) cache.get( session, DESCRIPTORS );
+ artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.get( session, ARTIFACT_POOL );
+ dependencies = (ConcurrentHashMap<Dependency, Dependency>) cache.get( session, DEPENDENCY_POOL );
+ descriptors = (ConcurrentHashMap<Object, Descriptor>) cache.get( session, DESCRIPTORS );
Review Comment:
Well, we could of course, but if you look at class
https://github.com/apache/maven-resolver/blob/master/maven-resolver-api/src/main/java/org/eclipse/aether/RepositoryCache.java
It is not marked as "noextend", hence it would be a breaking change => resolver major version. In fact, I think m2e does implement this class, this was one of the main reasons to introduce it AFAIR. This class is very differernt than SessionData, that is NOT meant to be implemented outside of resolver....
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas merged pull request #241: [MRESOLVER-320] Use strong references by default
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #241:
URL: https://github.com/apache/maven-resolver/pull/241
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #241: [MRESOLVER-320] Use strong references by default
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1105978948
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -53,6 +52,7 @@
*/
public final class DataPool
{
+ private static final String CONFIG_PROP_COLLECTOR_POOL_WEAK = "aether.dependencyCollector.pool.weak";
Review Comment:
That would also allow things like `lru(1024)` ... or whatever.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #241: [MRESOLVER-320] Use strong references
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1100319482
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -80,14 +76,14 @@ public DataPool( RepositorySystemSession session )
if ( cache != null )
{
- artifacts = (ObjectPool<Artifact>) cache.get( session, ARTIFACT_POOL );
- dependencies = (ObjectPool<Dependency>) cache.get( session, DEPENDENCY_POOL );
- descriptors = (Map<Object, WeakReference<Descriptor>>) cache.get( session, DESCRIPTORS );
+ artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.get( session, ARTIFACT_POOL );
+ dependencies = (ConcurrentHashMap<Dependency, Dependency>) cache.get( session, DEPENDENCY_POOL );
+ descriptors = (ConcurrentHashMap<Object, Descriptor>) cache.get( session, DESCRIPTORS );
Review Comment:
And with a default method in the interface ? `Map` as tons of them...
```
default Object computeIfAbsent( RepositorySystemSession session, Object key, Supplier<Object> supplier )
{
Object value = get( session, key );
if ( value == null )
{
value = supplier.get();
put( session, key, value );
}
return value;
}
```
This keeps compatibility. The default implementation should of course leverage the `cache.computeIfAbsent()` method of the underlying `ConcurrentHashMap`...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #241: [MRESOLVER-320] Use strong references
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1100290093
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -80,14 +76,14 @@ public DataPool( RepositorySystemSession session )
if ( cache != null )
{
- artifacts = (ObjectPool<Artifact>) cache.get( session, ARTIFACT_POOL );
- dependencies = (ObjectPool<Dependency>) cache.get( session, DEPENDENCY_POOL );
- descriptors = (Map<Object, WeakReference<Descriptor>>) cache.get( session, DESCRIPTORS );
+ artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.get( session, ARTIFACT_POOL );
+ dependencies = (ConcurrentHashMap<Dependency, Dependency>) cache.get( session, DEPENDENCY_POOL );
+ descriptors = (ConcurrentHashMap<Object, Descriptor>) cache.get( session, DESCRIPTORS );
Review Comment:
Could we add a `computeIfAbsent` method on the `RepositoryCache` to avoid this boilerplate ?
```
Object computeIfAbsent(@Nonnull Object key, @Nonnull Supplier<Object> supplier);
```
so that we can the do
```
artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.computeIfAbsent( session, ARTIFACT_POOL, () -> new ConcurrentHashMap<>() );
```
and remove the following 20 lines...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] gnodet commented on a diff in pull request #241: [MRESOLVER-320] Use strong references
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1100290093
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -80,14 +76,14 @@ public DataPool( RepositorySystemSession session )
if ( cache != null )
{
- artifacts = (ObjectPool<Artifact>) cache.get( session, ARTIFACT_POOL );
- dependencies = (ObjectPool<Dependency>) cache.get( session, DEPENDENCY_POOL );
- descriptors = (Map<Object, WeakReference<Descriptor>>) cache.get( session, DESCRIPTORS );
+ artifacts = (ConcurrentHashMap<Artifact, Artifact>) cache.get( session, ARTIFACT_POOL );
+ dependencies = (ConcurrentHashMap<Dependency, Dependency>) cache.get( session, DEPENDENCY_POOL );
+ descriptors = (ConcurrentHashMap<Object, Descriptor>) cache.get( session, DESCRIPTORS );
Review Comment:
Could we add a `computeIfAbsent` method on the `RepositoryCache` to avoid this boilerplate ?
```
Object computeIfAbsent(@Nonnull Object key, @Nonnull Supplier<Object> supplier);
```
so that we can the do
```
artifacts = cache.computeIfAbsent( session, ARTIFACT_POOL, () -> new ConcurrentHashMap<>() );
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven-resolver] cstamas commented on a diff in pull request #241: [MRESOLVER-320] Use strong references by default
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #241:
URL: https://github.com/apache/maven-resolver/pull/241#discussion_r1106003908
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/DataPool.java:
##########
@@ -415,4 +440,46 @@ public int hashCode()
return hashCode;
}
}
+
+ private interface InternPool<K, V>
+ {
+ V get( K key );
+
+ V intern( K key, V value );
+ }
+
+ private static class HardInternPool<K, V> implements InternPool<K, V>
+ {
+ private final ConcurrentHashMap<K, V> map = new ConcurrentHashMap<>( 256 );
+
+ @Override
+ public V get( K key )
+ {
+ return map.get( key );
+ }
+
+ @Override
+ public V intern( K key, V value )
+ {
+ return map.computeIfAbsent( key, k -> value );
+ }
+ }
+
+ private static class WeakInternPool<K, V> implements InternPool<K, V>
+ {
+ private final WeakHashMap<K, WeakReference<V>> map = new WeakHashMap<>( 256 );
+
+ @Override
+ public V get( K key )
Review Comment:
fixed (wrapping it instead)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org