You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/11/02 14:20:49 UTC
[GitHub] [maven-resolver] cstamas opened a new pull request, #213: Shared executor
cstamas opened a new pull request, #213:
URL: https://github.com/apache/maven-resolver/pull/213
Now that we have shutdown hook, and context parameters, this is possible.
Simplest possible but shared executor implementation.
--
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] michael-o commented on a diff in pull request #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1014828015
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultMetadataResolver.java:
##########
@@ -374,41 +391,36 @@ else if ( exception == null )
if ( !tasks.isEmpty() )
{
- int threads = ConfigUtils.getInteger( session, 4, CONFIG_PROP_THREADS );
- Executor executor = getExecutor( Math.min( tasks.size(), threads ) );
- try
+ RunnableErrorForwarder errorForwarder = new RunnableErrorForwarder();
+ ArrayList<Runnable> runnable = new ArrayList<>( tasks.size() );
Review Comment:
runnables
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -146,8 +150,10 @@
this.repository = repository;
this.fileProcessor = fileProcessor;
this.providedChecksumsSources = providedChecksumsSources;
+ this.resolverExecutor = resolverExecutorService.getResolverExecutor( session, RepositoryConnector.class,
+ ConfigUtils.getInteger( session, CONFIG_PROP_THREADS_DEFAULT, CONFIG_PROP_THREADS,
+ "maven.artifact.threads" ) );
Review Comment:
We should also deprecate this property since it does not reflect reality. MD is not artifacts, but this connector does both.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutorService.java:
##########
@@ -0,0 +1,92 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.concurrency.ResolverExecutorService;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * This implementation uses {@link RepositorySystemSession#getData()} to store created {@link ExecutorService}
+ * instances. It creates instances that may be eventually garbage collected, so no explicit shutdown happens on
+ * them. When {@code maxThreads} parameter is 1 (accepted values are greater than zero), this implementation assumes
+ * caller wants "direct execution" (on caller thread) and creates {@link ResolverExecutor} instances accordingly.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutorService implements ResolverExecutorService
+{
+ @Override
+ public ResolverExecutor getResolverExecutor( RepositorySystemSession session,
+ Class<?> service,
+ int maxThreads )
+ {
+ requireNonNull( session );
+ requireNonNull( service );
+ if ( maxThreads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+
+ final ExecutorService executorService;
+ if ( maxThreads == 1 ) // direct
+ {
+ executorService = null;
+ }
+ else // shared && pooled
+ {
+ String key = DefaultResolverExecutorService.class.getName() + "." + service.getSimpleName();
+ executorService = (ExecutorService) session.getData()
+ .computeIfAbsent( key, () -> createExecutorService( service, maxThreads ) );
+ }
+ return new DefaultResolverExecutor( executorService );
+ }
+
+ /**
+ * Creates am {@link ExecutorService} that allows its core threads to die off in case of inactivity, and allows
+ * for proper garbage collection. This is important detail, as these instances are kept within session data, and
+ * currently there is no way to shut down them.
+ */
+ private ExecutorService createExecutorService( Class<?> service, int maxThreads )
+ {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(
+ maxThreads,
+ maxThreads,
+ 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(),
+ new WorkerThreadFactory( getClass().getSimpleName() + "-" + service.getSimpleName() + "-" )
+ );
+ threadPoolExecutor.allowCoreThreadTimeOut( true );
Review Comment:
Why is this necessary?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutorService.java:
##########
@@ -0,0 +1,92 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.concurrency.ResolverExecutorService;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * This implementation uses {@link RepositorySystemSession#getData()} to store created {@link ExecutorService}
+ * instances. It creates instances that may be eventually garbage collected, so no explicit shutdown happens on
+ * them. When {@code maxThreads} parameter is 1 (accepted values are greater than zero), this implementation assumes
+ * caller wants "direct execution" (on caller thread) and creates {@link ResolverExecutor} instances accordingly.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutorService implements ResolverExecutorService
+{
+ @Override
+ public ResolverExecutor getResolverExecutor( RepositorySystemSession session,
+ Class<?> service,
+ int maxThreads )
+ {
+ requireNonNull( session );
+ requireNonNull( service );
+ if ( maxThreads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+
+ final ExecutorService executorService;
+ if ( maxThreads == 1 ) // direct
+ {
+ executorService = null;
+ }
+ else // shared && pooled
+ {
+ String key = DefaultResolverExecutorService.class.getName() + "." + service.getSimpleName();
+ executorService = (ExecutorService) session.getData()
+ .computeIfAbsent( key, () -> createExecutorService( service, maxThreads ) );
+ }
+ return new DefaultResolverExecutor( executorService );
+ }
+
+ /**
+ * Creates am {@link ExecutorService} that allows its core threads to die off in case of inactivity, and allows
+ * for proper garbage collection. This is important detail, as these instances are kept within session data, and
+ * currently there is no way to shut down them.
+ */
+ private ExecutorService createExecutorService( Class<?> service, int maxThreads )
+ {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(
+ maxThreads,
+ maxThreads,
+ 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(),
+ new WorkerThreadFactory( getClass().getSimpleName() + "-" + service.getSimpleName() + "-" )
Review Comment:
So we have lost the per repo thread pool?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultMetadataResolver.java:
##########
@@ -83,8 +78,16 @@
implements MetadataResolver, Service
{
+ /**
+ * The count of threads to be used when resolving metadata in parallel, default value 4.
+ */
private static final String CONFIG_PROP_THREADS = "aether.metadataResolver.threads";
+ /**
+ * The default value for {@link #CONFIG_PROP_THREADS}.
+ */
+ private static final int CONFIG_PROP_THREADS_DEFAULT = 4;
Review Comment:
Why not 5 like in the other one?
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -287,9 +269,10 @@ public void get( Collection<? extends ArtifactDownload> artifactDownloads,
task = new GetTaskRunner( location, transfer.getFile(), checksumPolicy,
checksumAlgorithmFactories, checksumLocations, providedChecksums, listener );
}
- executor.execute( errorForwarder.wrap( task ) );
+ runnable.add( errorForwarder.wrap( task ) );
}
+ resolverExecutor.submitOrDirect( runnable );
Review Comment:
Why wait until they are all collected instead separate MD and A like before?
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutorService.java:
##########
@@ -0,0 +1,92 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.concurrency.ResolverExecutorService;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * This implementation uses {@link RepositorySystemSession#getData()} to store created {@link ExecutorService}
+ * instances. It creates instances that may be eventually garbage collected, so no explicit shutdown happens on
+ * them. When {@code maxThreads} parameter is 1 (accepted values are greater than zero), this implementation assumes
+ * caller wants "direct execution" (on caller thread) and creates {@link ResolverExecutor} instances accordingly.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutorService implements ResolverExecutorService
+{
+ @Override
+ public ResolverExecutor getResolverExecutor( RepositorySystemSession session,
+ Class<?> service,
+ int maxThreads )
+ {
+ requireNonNull( session );
+ requireNonNull( service );
+ if ( maxThreads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
Review Comment:
maxThreads must be...
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,93 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * It relies on ctor passed {@link ExecutorService} that may be {@code null}, in which case "direct invocation" (on
+ * caller thread) happens, otherwise the non-null executor service is used.
+ */
+final class DefaultResolverExecutor implements ResolverExecutor
+{
+ private final ExecutorService executorService;
+
+ DefaultResolverExecutor( final ExecutorService executorService )
+ {
+ this.executorService = executorService;
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
Review Comment:
I think from an abstract PoV just `submit` is enough. It is an implementation detail whether it should be executed directly or scheduled for execution. The behavior should be described in the Javadoc of the class.
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -225,11 +205,13 @@ public void get( Collection<? extends ArtifactDownload> artifactDownloads,
throw new IllegalStateException( "connector closed" );
}
- Executor executor = getExecutor( artifactDownloads, metadataDownloads );
RunnableErrorForwarder errorForwarder = new RunnableErrorForwarder();
List<ChecksumAlgorithmFactory> checksumAlgorithmFactories = layout.getChecksumAlgorithmFactories();
+ Collection<? extends MetadataDownload> mds = safe( metadataDownloads );
+ Collection<? extends ArtifactDownload> ads = safe( artifactDownloads );
+ ArrayList<Runnable> runnable = new ArrayList<>( mds.size() + ads.size() );
Review Comment:
runnables
--
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 #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012585299
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,115 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ *
+ * Creates a single shared {@link ExecutorService} instance that is shut along with repository system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "maven.artifact.threads";
+
+ private static final int DEFAULT_THREADS = 5;
+
+ private final ExecutorService executorService;
+
+ /**
+ * SL ctor.
+ *
+ * @deprecated For use in SL only.
+ */
+ @Deprecated
+ public DefaultResolverExecutor()
+ {
+ this.executorService = new ThreadPoolExecutor( DEFAULT_THREADS, DEFAULT_THREADS, 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(), new WorkerThreadFactory( getClass().getSimpleName() + '-' ) );
+ }
+
+ @Inject
+ public DefaultResolverExecutor( @Named( "${" + CONF_PROP_THREADS + ":-" + DEFAULT_THREADS + "}" ) final int threads,
+ RepositorySystemLifecycle lifecycle )
+ {
+ if ( threads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be grater than zero" );
Review Comment:
fixed
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1015198784
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultMetadataResolver.java:
##########
@@ -83,8 +78,16 @@
implements MetadataResolver, Service
{
+ /**
+ * The count of threads to be used when resolving metadata in parallel, default value 4.
+ */
private static final String CONFIG_PROP_THREADS = "aether.metadataResolver.threads";
+ /**
+ * The default value for {@link #CONFIG_PROP_THREADS}.
+ */
+ private static final int CONFIG_PROP_THREADS_DEFAULT = 4;
Review Comment:
I kept same values as before change, this component used by default 4 threads, no change in that area.
--
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] michael-o commented on pull request #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#issuecomment-1303076477
Will try to review today
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1015200916
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutorService.java:
##########
@@ -0,0 +1,92 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.concurrency.ResolverExecutorService;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * This implementation uses {@link RepositorySystemSession#getData()} to store created {@link ExecutorService}
+ * instances. It creates instances that may be eventually garbage collected, so no explicit shutdown happens on
+ * them. When {@code maxThreads} parameter is 1 (accepted values are greater than zero), this implementation assumes
+ * caller wants "direct execution" (on caller thread) and creates {@link ResolverExecutor} instances accordingly.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutorService implements ResolverExecutorService
+{
+ @Override
+ public ResolverExecutor getResolverExecutor( RepositorySystemSession session,
+ Class<?> service,
+ int maxThreads )
+ {
+ requireNonNull( session );
+ requireNonNull( service );
+ if ( maxThreads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+
+ final ExecutorService executorService;
+ if ( maxThreads == 1 ) // direct
+ {
+ executorService = null;
+ }
+ else // shared && pooled
+ {
+ String key = DefaultResolverExecutorService.class.getName() + "." + service.getSimpleName();
+ executorService = (ExecutorService) session.getData()
+ .computeIfAbsent( key, () -> createExecutorService( service, maxThreads ) );
+ }
+ return new DefaultResolverExecutor( executorService );
+ }
+
+ /**
+ * Creates am {@link ExecutorService} that allows its core threads to die off in case of inactivity, and allows
+ * for proper garbage collection. This is important detail, as these instances are kept within session data, and
+ * currently there is no way to shut down them.
+ */
+ private ExecutorService createExecutorService( Class<?> service, int maxThreads )
+ {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(
+ maxThreads,
+ maxThreads,
+ 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(),
+ new WorkerThreadFactory( getClass().getSimpleName() + "-" + service.getSimpleName() + "-" )
+ );
+ threadPoolExecutor.allowCoreThreadTimeOut( true );
Review Comment:
To let the pool become garbage collected, otherwise (if have living threads) it will NOT be GC-ed. OTOH, for performance reasons, we do want to create pool (and keep the pool) with live threads for some....
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutorService.java:
##########
@@ -0,0 +1,92 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.concurrency.ResolverExecutorService;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * This implementation uses {@link RepositorySystemSession#getData()} to store created {@link ExecutorService}
+ * instances. It creates instances that may be eventually garbage collected, so no explicit shutdown happens on
+ * them. When {@code maxThreads} parameter is 1 (accepted values are greater than zero), this implementation assumes
+ * caller wants "direct execution" (on caller thread) and creates {@link ResolverExecutor} instances accordingly.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutorService implements ResolverExecutorService
+{
+ @Override
+ public ResolverExecutor getResolverExecutor( RepositorySystemSession session,
+ Class<?> service,
+ int maxThreads )
+ {
+ requireNonNull( session );
+ requireNonNull( service );
+ if ( maxThreads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+
+ final ExecutorService executorService;
+ if ( maxThreads == 1 ) // direct
+ {
+ executorService = null;
+ }
+ else // shared && pooled
+ {
+ String key = DefaultResolverExecutorService.class.getName() + "." + service.getSimpleName();
+ executorService = (ExecutorService) session.getData()
+ .computeIfAbsent( key, () -> createExecutorService( service, maxThreads ) );
+ }
+ return new DefaultResolverExecutor( executorService );
+ }
+
+ /**
+ * Creates am {@link ExecutorService} that allows its core threads to die off in case of inactivity, and allows
+ * for proper garbage collection. This is important detail, as these instances are kept within session data, and
+ * currently there is no way to shut down them.
+ */
+ private ExecutorService createExecutorService( Class<?> service, int maxThreads )
+ {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(
+ maxThreads,
+ maxThreads,
+ 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(),
+ new WorkerThreadFactory( getClass().getSimpleName() + "-" + service.getSimpleName() + "-" )
Review Comment:
Good point, fixing
--
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] caiwei-ebay commented on a diff in pull request #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012801556
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,174 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * If configured threads count is 1, direct invocation happens, otherwise a thread pool ({@link ExecutorService}) is
+ * being used. If needed, creates a single shared {@link ExecutorService} instance, that is shut along with repository
+ * system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "aether.executor.threads";
Review Comment:
maven.artifact.threads renamed to aether.executor.threads, this could be incompatible change as the former is a widely used parameter.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,174 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * If configured threads count is 1, direct invocation happens, otherwise a thread pool ({@link ExecutorService}) is
+ * being used. If needed, creates a single shared {@link ExecutorService} instance, that is shut along with repository
+ * system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "aether.executor.threads";
+
+ private static final int DEFAULT_THREADS = 5;
+
+ private final ExecutorService executorService;
+
+ private final int threads;
+
+ /**
+ * SL ctor.
+ *
+ * @deprecated For use in SL only.
+ */
+ @Deprecated
+ public DefaultResolverExecutor()
+ {
+ this.threads = DEFAULT_THREADS;
+ this.executorService = createExecutorService( threads );
+ }
+
+ @Inject
+ public DefaultResolverExecutor( @Named( "${" + CONF_PROP_THREADS + ":-" + DEFAULT_THREADS + "}" ) final int threads,
+ RepositorySystemLifecycle lifecycle )
+ {
+ if ( threads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+ requireNonNull( lifecycle );
+ this.threads = threads;
+ this.executorService = createExecutorService( threads );
+ lifecycle.addOnSystemEndedHandler( this::shutdown );
+ }
+
+ private ExecutorService createExecutorService( int threads )
+ {
+ if ( threads == 1 )
+ {
+ return null; // we use direct execution
+ }
+ else
+ {
+ return new ThreadPoolExecutor( 0, threads, 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(), new WorkerThreadFactory( getClass().getSimpleName() + '-' ) );
+ }
+ }
+
+ @Override
+ public void initService( ServiceLocator locator )
+ {
+ locator.getService( RepositorySystemLifecycle.class ).addOnSystemEndedHandler( this::shutdown );
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
+ {
+ requireNonNull( tasks );
Review Comment:
When resolves a snapshot dependency or dependency with version range , Maven resolves metadata.xml from multiple repositories in parallel: https://github.com/apache/maven/blob/master/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java#L158
Suppose we have multiple repositories like releases, snapshots, commercial, legacy, testing in our private Maven nexus repository and we need to resolve 5 dependencies (snapshot or with version range) in parallel.
When resolve with BFDependencyCollector, 5 tasks to resolve each dependency have used up all 5 threads, and due to resolving each dependency (task A) need to resolve metadata (task B), the metadata resolution task B will be also submitted to the same executor. A depends on B but B has to wait available threads. I think this could be lead to endless waiting... No?
As it is better to use separate thread pool for separate tasks, maybe the SharedExecutor here should have separate pool for separate tasks? At least, all poms downloading used one pool and all metadata.xml downloading used another pool. Please advice.
-
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1015226766
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,93 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * It relies on ctor passed {@link ExecutorService} that may be {@code null}, in which case "direct invocation" (on
+ * caller thread) happens, otherwise the non-null executor service is used.
+ */
+final class DefaultResolverExecutor implements ResolverExecutor
+{
+ private final ExecutorService executorService;
+
+ DefaultResolverExecutor( final ExecutorService executorService )
+ {
+ this.executorService = executorService;
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
Review Comment:
renamed to `submitBatch`
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutorService.java:
##########
@@ -0,0 +1,92 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.concurrency.ResolverExecutorService;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * This implementation uses {@link RepositorySystemSession#getData()} to store created {@link ExecutorService}
+ * instances. It creates instances that may be eventually garbage collected, so no explicit shutdown happens on
+ * them. When {@code maxThreads} parameter is 1 (accepted values are greater than zero), this implementation assumes
+ * caller wants "direct execution" (on caller thread) and creates {@link ResolverExecutor} instances accordingly.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutorService implements ResolverExecutorService
+{
+ @Override
+ public ResolverExecutor getResolverExecutor( RepositorySystemSession session,
+ Class<?> service,
+ int maxThreads )
+ {
+ requireNonNull( session );
+ requireNonNull( service );
+ if ( maxThreads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
Review Comment:
Fixed
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#issuecomment-1306915519
> Sharing executors must be done with caution.
>
> Is there any way that the executor may be running out of threads to service a call ? For example, resolving multiple files in parallel, each file being resolved in its own thread, then downloading dependencies and running out of threads ?
>
> I think using a more general `ForkJoinPool` which supports work stealing would be safer to avoid running into such problems.
I kinda agree with @gnodet here, and will "tone down" this PR to NOT SHARE executors, merely will introduce this facade but instances will be handled as before (as even now, it is not 100% what it was before: collector had per arg instance, now shared). And I cannot certainly asnwer the question "Is there any way that the executor may be running out of threads to service a call".
OTOH, I disagree with forkjoinpool: all these threads will do (probably blocking, maybe inifinite) HTTP IO, not to mention the refactoring needed to use them (most of "client" code remained unchanged here, using plain Runnable and Callable).
---
Hence, this PR will change to merely "centralize executor creation and (probably) use ResolverExecutor shim instead directly exposing ExecutorService iface"...
--
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] caiwei-ebay commented on pull request #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#issuecomment-1301528882
@cstamas
The PR looks good to me. Thanks for the quick fix.
My main concern is about how to tell user pom/metadata.xml is downloaded in parallel now with BF collector.
As the special param "aether.dependencyCollector.bf.threads" has to be removed using SharedExecutor, could you put some lines to explain the parallel download behavior in param "aether.dependencyCollector.impl" where you've introduced BF and DF. With such information, user may try the BF and feedback so we can discuss whether BF could be the default based on the feedbacks.
--
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 #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#issuecomment-1302337949
Reworked to:
* have split pools (service can decide using service "key")
* pools are stored in session, so each session has own pool(s)
* this merely reflects what was before, but all is centralized.
--
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] michael-o commented on a diff in pull request #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1014829852
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,174 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * If configured threads count is 1, direct invocation happens, otherwise a thread pool ({@link ExecutorService}) is
+ * being used. If needed, creates a single shared {@link ExecutorService} instance, that is shut along with repository
+ * system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "aether.executor.threads";
+
+ private static final int DEFAULT_THREADS = 5;
+
+ private final ExecutorService executorService;
+
+ private final int threads;
+
+ /**
+ * SL ctor.
+ *
+ * @deprecated For use in SL only.
+ */
+ @Deprecated
+ public DefaultResolverExecutor()
+ {
+ this.threads = DEFAULT_THREADS;
+ this.executorService = createExecutorService( threads );
+ }
+
+ @Inject
+ public DefaultResolverExecutor( @Named( "${" + CONF_PROP_THREADS + ":-" + DEFAULT_THREADS + "}" ) final int threads,
+ RepositorySystemLifecycle lifecycle )
+ {
+ if ( threads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+ requireNonNull( lifecycle );
+ this.threads = threads;
+ this.executorService = createExecutorService( threads );
+ lifecycle.addOnSystemEndedHandler( this::shutdown );
+ }
+
+ private ExecutorService createExecutorService( int threads )
+ {
+ if ( threads == 1 )
+ {
+ return null; // we use direct execution
+ }
+ else
+ {
+ return new ThreadPoolExecutor( 0, threads, 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(), new WorkerThreadFactory( getClass().getSimpleName() + '-' ) );
+ }
+ }
+
+ @Override
+ public void initService( ServiceLocator locator )
+ {
+ locator.getService( RepositorySystemLifecycle.class ).addOnSystemEndedHandler( this::shutdown );
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
+ {
+ requireNonNull( tasks );
Review Comment:
I'd like also add as I understand the artifact thread pools where per repo, one this is one per all repos?
I think we need a overview what executor services where created previously and which now to compare whether this could create a bottleneck.
--
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] caiwei-ebay commented on a diff in pull request #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012476849
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,115 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ *
+ * Creates a single shared {@link ExecutorService} instance that is shut along with repository system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "maven.artifact.threads";
+
+ private static final int DEFAULT_THREADS = 5;
+
+ private final ExecutorService executorService;
+
+ /**
+ * SL ctor.
+ *
+ * @deprecated For use in SL only.
+ */
+ @Deprecated
+ public DefaultResolverExecutor()
+ {
+ this.executorService = new ThreadPoolExecutor( DEFAULT_THREADS, DEFAULT_THREADS, 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(), new WorkerThreadFactory( getClass().getSimpleName() + '-' ) );
+ }
+
+ @Inject
+ public DefaultResolverExecutor( @Named( "${" + CONF_PROP_THREADS + ":-" + DEFAULT_THREADS + "}" ) final int threads,
+ RepositorySystemLifecycle lifecycle )
+ {
+ if ( threads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be grater than zero" );
Review Comment:
typo grator -> greator
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1015198275
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -146,8 +150,10 @@
this.repository = repository;
this.fileProcessor = fileProcessor;
this.providedChecksumsSources = providedChecksumsSources;
+ this.resolverExecutor = resolverExecutorService.getResolverExecutor( session, RepositoryConnector.class,
+ ConfigUtils.getInteger( session, CONFIG_PROP_THREADS_DEFAULT, CONFIG_PROP_THREADS,
+ "maven.artifact.threads" ) );
Review Comment:
Maybe we should just remove it? We are going for 1.9 after all....
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1015199836
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,93 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * It relies on ctor passed {@link ExecutorService} that may be {@code null}, in which case "direct invocation" (on
+ * caller thread) happens, otherwise the non-null executor service is used.
+ */
+final class DefaultResolverExecutor implements ResolverExecutor
+{
+ private final ExecutorService executorService;
+
+ DefaultResolverExecutor( final ExecutorService executorService )
+ {
+ this.executorService = executorService;
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
Review Comment:
I wanted to clearly distinquish, that's all, moreover, if `submit` then we overload the other method as well.... but right, semantics should not be exposed, so maybe `submitBatch`?
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1015202072
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -287,9 +269,10 @@ public void get( Collection<? extends ArtifactDownload> artifactDownloads,
task = new GetTaskRunner( location, transfer.getFile(), checksumPolicy,
checksumAlgorithmFactories, checksumLocations, providedChecksums, listener );
}
- executor.execute( errorForwarder.wrap( task ) );
+ runnable.add( errorForwarder.wrap( task ) );
}
+ resolverExecutor.submitOrDirect( runnable );
Review Comment:
What is the difference? It's not this line that wait but next, the unchanged one.
--
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] caiwei-ebay commented on a diff in pull request #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012891403
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,174 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * If configured threads count is 1, direct invocation happens, otherwise a thread pool ({@link ExecutorService}) is
+ * being used. If needed, creates a single shared {@link ExecutorService} instance, that is shut along with repository
+ * system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "aether.executor.threads";
+
+ private static final int DEFAULT_THREADS = 5;
+
+ private final ExecutorService executorService;
+
+ private final int threads;
+
+ /**
+ * SL ctor.
+ *
+ * @deprecated For use in SL only.
+ */
+ @Deprecated
+ public DefaultResolverExecutor()
+ {
+ this.threads = DEFAULT_THREADS;
+ this.executorService = createExecutorService( threads );
+ }
+
+ @Inject
+ public DefaultResolverExecutor( @Named( "${" + CONF_PROP_THREADS + ":-" + DEFAULT_THREADS + "}" ) final int threads,
+ RepositorySystemLifecycle lifecycle )
+ {
+ if ( threads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+ requireNonNull( lifecycle );
+ this.threads = threads;
+ this.executorService = createExecutorService( threads );
+ lifecycle.addOnSystemEndedHandler( this::shutdown );
+ }
+
+ private ExecutorService createExecutorService( int threads )
+ {
+ if ( threads == 1 )
+ {
+ return null; // we use direct execution
+ }
+ else
+ {
+ return new ThreadPoolExecutor( 0, threads, 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(), new WorkerThreadFactory( getClass().getSimpleName() + '-' ) );
+ }
+ }
+
+ @Override
+ public void initService( ServiceLocator locator )
+ {
+ locator.getService( RepositorySystemLifecycle.class ).addOnSystemEndedHandler( this::shutdown );
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
+ {
+ requireNonNull( tasks );
Review Comment:
When resolves a snapshot dependency or dependency with version range, Maven resolves metadata.xml from multiple repositories in parallel: https://github.com/apache/maven/blob/master/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java#L158
With original design, metadata.xml is using a separate pool, pom or jar can use RepositoryConnector's pool but pom downloading is in serial while jar is in parallel:
- metadata.xml use a separate pool to download metadata.xml from multiple repositories, each downloading calls RepositoryConnector's **get(null, Collections.singleList(MetadataDownload))**, it actually uses DirectExecutor without leveraging RepositoryConnector's thread pool;
- pom downloading directly calls RepositoryConnector's get(Collections.singleList(ArtifactDownload), null), so it also uses DirectExecutor without leveraging RepositoryConnector's thread pool;
- jar downloading calls RepositoryConnector's get(List(ArtifactDownload), null), it do leverage RepositoryConnector's thread pool.
With this approach, metadata, pom and jar all share one pool:
Suppose we have multiple repositories like releases, snapshots, commercial, legacy, testing, etc. in our private Maven nexus repository and we need to resolve 5 dependencies (snapshot or with version range) in parallel.
When resolve with BFDependencyCollector, 5 tasks to resolve each dependency have used up all 5 threads, and due to resolving each dependency (let's name with task A) need to resolve metadata (task B), the metadata resolution task B will be also submitted to the same pool. Here A depends on B but B has to wait available threads. I think this could be lead to endless waiting, right?
As it is better to use separate thread pool for separate tasks, maybe the SharedExecutor here should have separate pool for separate tasks? At least, pom/jar downloading and BF collect (it also need to download pom but pom downloading actually uses directlyExecute with your recent fix, thus safe to use the same pool) can use one pool and metadata.xml downloading still use another pool? Please advice.
--
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] caiwei-ebay commented on a diff in pull request #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012479745
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -656,18 +622,4 @@ private void uploadChecksum( URI location, Object checksum )
}
}
-
- private static class DirectExecutor
- implements Executor
- {
-
- static final Executor INSTANCE = new DirectExecutor();
-
- @Override
- public void execute( Runnable command )
- {
- command.run();
- }
-
Review Comment:
Given this being deleted, I think it may introduce additional cost for DF.
DF downloads pom one by one, originally it will be downloaded by main thread directly and now it will be downloaded in a thread and the main thread has to wait
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java:
##########
@@ -475,23 +482,22 @@ else if ( descriptorResult == DataPool.NO_DESCRIPTOR )
static class ParallelDescriptorResolver
{
- final ExecutorService executorService;
+ final ResolverExecutor executor;
/**
* Artifact ID -> Future of DescriptorResolutionResult
*/
final Map<String, Future<DescriptorResolutionResult>> results = new ConcurrentHashMap<>( 256 );
- final Logger logger = LoggerFactory.getLogger( getClass() );
- ParallelDescriptorResolver( RepositorySystemSession session )
+ ParallelDescriptorResolver( ResolverExecutor executor )
{
- this.executorService = getExecutorService( session );
+ this.executor = executor;
}
void resolveDescriptors( Artifact artifact, Callable<DescriptorResolutionResult> callable )
{
results.computeIfAbsent( ArtifactIdUtils.toId( artifact ),
- key -> this.executorService.submit( callable ) );
+ key -> this.executor.submit( callable ) );
Review Comment:
In the illustration below:
https://camo.githubusercontent.com/71a9619274e478245df1c37423239b79f6b3036a3af72690d7ace08419ee21ac/68747470733a2f2f6973737565732e6170616368652e6f72672f6a6972612f7365637572652f6174746163686d656e742f31333034373931392f7265736f6c76655f646570732e706e67
As we submit a Callable<DescriptorResolutionResult> here, and this callable finally resorts to RepositoryConnector which will then submit a Collection<Artifact> (a collection with single artifact) to the executor, 2 different tasks running in one executor and besides the 2 tasks has dependencies.
The original behavior is:
we submit a Callable<DescriptorResolutionResult> here, and this callable finally resorts to RepositoryConnector which will then submit a Collection<Artifact> (a collection with single artifact) to the executor, but this time the executor is actually a DirectExecutor which actually runs in main thread. Please check code below.
if ( maxThreads <= 1 )
{
return DirectExecutor.INSTANCE;
}
int tasks = safe( artifacts ).size() + safe( metadatas ).size();
if ( tasks <= 1 )
{
return DirectExecutor.INSTANCE;
}
##########
src/site/markdown/configuration.md:
##########
@@ -52,7 +52,6 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
`aether.dependencyCollector.maxExceptions` | int | Only exceptions up to the number given in this configuration property are emitted. Exceptions which exceed that number are swallowed. | `50` | no
`aether.dependencyCollector.impl` | String | The name of the dependency collector implementation to use: depth-first (original) named `df`, and breadth-first (new in 1.8.0) named `bf`. Both collectors produce equivalent results, but they may differ performance wise, depending on project being applied to. Our experience shows that existing `df` is well suited for smaller to medium size projects, while `bf` may perform better on huge projects with many dependencies. Experiment (and come back to us!) to figure out which one suits you the better. | `"df"` | no
Review Comment:
Shall we add more info about "BF will download poms/metadata.xml in parallel"?
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1015227078
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutorService.java:
##########
@@ -0,0 +1,92 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.RepositorySystemSession;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.concurrency.ResolverExecutorService;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * This implementation uses {@link RepositorySystemSession#getData()} to store created {@link ExecutorService}
+ * instances. It creates instances that may be eventually garbage collected, so no explicit shutdown happens on
+ * them. When {@code maxThreads} parameter is 1 (accepted values are greater than zero), this implementation assumes
+ * caller wants "direct execution" (on caller thread) and creates {@link ResolverExecutor} instances accordingly.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutorService implements ResolverExecutorService
+{
+ @Override
+ public ResolverExecutor getResolverExecutor( RepositorySystemSession session,
+ Class<?> service,
+ int maxThreads )
+ {
+ requireNonNull( session );
+ requireNonNull( service );
+ if ( maxThreads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+
+ final ExecutorService executorService;
+ if ( maxThreads == 1 ) // direct
+ {
+ executorService = null;
+ }
+ else // shared && pooled
+ {
+ String key = DefaultResolverExecutorService.class.getName() + "." + service.getSimpleName();
+ executorService = (ExecutorService) session.getData()
+ .computeIfAbsent( key, () -> createExecutorService( service, maxThreads ) );
+ }
+ return new DefaultResolverExecutor( executorService );
+ }
+
+ /**
+ * Creates am {@link ExecutorService} that allows its core threads to die off in case of inactivity, and allows
+ * for proper garbage collection. This is important detail, as these instances are kept within session data, and
+ * currently there is no way to shut down them.
+ */
+ private ExecutorService createExecutorService( Class<?> service, int maxThreads )
+ {
+ ThreadPoolExecutor threadPoolExecutor = new ThreadPoolExecutor(
+ maxThreads,
+ maxThreads,
+ 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(),
+ new WorkerThreadFactory( getClass().getSimpleName() + "-" + service.getSimpleName() + "-" )
Review Comment:
Fixed
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#issuecomment-1306991904
Ok, now I feel I should not push this anymore, as I agree now with use of forkJoinPool. Hence, will close this PR, as that change would be bigger, but I want 1.9.0 out as soon as possible....
--
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] caiwei-ebay commented on a diff in pull request #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
caiwei-ebay commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012891403
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,174 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * If configured threads count is 1, direct invocation happens, otherwise a thread pool ({@link ExecutorService}) is
+ * being used. If needed, creates a single shared {@link ExecutorService} instance, that is shut along with repository
+ * system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "aether.executor.threads";
+
+ private static final int DEFAULT_THREADS = 5;
+
+ private final ExecutorService executorService;
+
+ private final int threads;
+
+ /**
+ * SL ctor.
+ *
+ * @deprecated For use in SL only.
+ */
+ @Deprecated
+ public DefaultResolverExecutor()
+ {
+ this.threads = DEFAULT_THREADS;
+ this.executorService = createExecutorService( threads );
+ }
+
+ @Inject
+ public DefaultResolverExecutor( @Named( "${" + CONF_PROP_THREADS + ":-" + DEFAULT_THREADS + "}" ) final int threads,
+ RepositorySystemLifecycle lifecycle )
+ {
+ if ( threads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+ requireNonNull( lifecycle );
+ this.threads = threads;
+ this.executorService = createExecutorService( threads );
+ lifecycle.addOnSystemEndedHandler( this::shutdown );
+ }
+
+ private ExecutorService createExecutorService( int threads )
+ {
+ if ( threads == 1 )
+ {
+ return null; // we use direct execution
+ }
+ else
+ {
+ return new ThreadPoolExecutor( 0, threads, 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(), new WorkerThreadFactory( getClass().getSimpleName() + '-' ) );
+ }
+ }
+
+ @Override
+ public void initService( ServiceLocator locator )
+ {
+ locator.getService( RepositorySystemLifecycle.class ).addOnSystemEndedHandler( this::shutdown );
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
+ {
+ requireNonNull( tasks );
Review Comment:
When resolves a snapshot dependency or dependency with version range, Maven resolves metadata.xml from multiple repositories in parallel: https://github.com/apache/maven/blob/master/maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/DefaultVersionRangeResolver.java#L158
With original design, metadata.xml is using a separate pool, pom or jar can use RepositoryConnector's pool but pom downloading is in serial while jar is in parallel:
- metadata.xml use a separate pool to download metadata.xml from multiple repositories, each downloading calls RepositoryConnector's **get(null, Collections.singleList(MetadataDownload))**, it actually uses DirectExecutor without leveraging RepositoryConnector's thread pool;
- pom downloading directly calls RepositoryConnector's get(Collections.singleList(ArtifactDownload), null), so it also uses DirectExecutor without leveraging RepositoryConnector's thread pool;
- jar downloading calls RepositoryConnector's get(List(ArtifactDownload), null), it do leverage RepositoryConnector's thread pool.
With this approach, metadata, pom and jar all share one pool:
Suppose we have multiple repositories like releases, snapshots, commercial, legacy, testing, etc. in our private Maven nexus repository and we need to resolve 5 dependencies (snapshot or with version range) in parallel.
When resolve with BFDependencyCollector, 5 tasks to resolve each dependency have used up all 5 threads, and due to resolving each dependency (let's name with task A) need to resolve metadata (task B), the metadata resolution task B will be also submitted to the same pool. Here A depends on B but B has to wait available threads. I think this could be lead to endless waiting, right?
As it is better to use separate thread pool for separate tasks, maybe the SharedExecutor here should have separate pool for separate tasks? At least, BF collect and pom/jar downloading can use one pool and metadata.xml downloading still use another pool? Please advice.
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
gnodet commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1016361394
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/concurrency/DefaultResolverExecutor.java:
##########
@@ -0,0 +1,174 @@
+package org.eclipse.aether.internal.impl.concurrency;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Collection;
+import java.util.concurrent.Callable;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
+
+import org.eclipse.aether.impl.RepositorySystemLifecycle;
+import org.eclipse.aether.spi.concurrency.ResolverExecutor;
+import org.eclipse.aether.spi.locator.Service;
+import org.eclipse.aether.spi.locator.ServiceLocator;
+import org.eclipse.aether.util.concurrency.WorkerThreadFactory;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Default implementation of {@link ResolverExecutor}.
+ * <p>
+ * If configured threads count is 1, direct invocation happens, otherwise a thread pool ({@link ExecutorService}) is
+ * being used. If needed, creates a single shared {@link ExecutorService} instance, that is shut along with repository
+ * system.
+ */
+@Singleton
+@Named
+public final class DefaultResolverExecutor implements ResolverExecutor, Service
+{
+ private static final String CONF_PROP_THREADS = "aether.executor.threads";
+
+ private static final int DEFAULT_THREADS = 5;
+
+ private final ExecutorService executorService;
+
+ private final int threads;
+
+ /**
+ * SL ctor.
+ *
+ * @deprecated For use in SL only.
+ */
+ @Deprecated
+ public DefaultResolverExecutor()
+ {
+ this.threads = DEFAULT_THREADS;
+ this.executorService = createExecutorService( threads );
+ }
+
+ @Inject
+ public DefaultResolverExecutor( @Named( "${" + CONF_PROP_THREADS + ":-" + DEFAULT_THREADS + "}" ) final int threads,
+ RepositorySystemLifecycle lifecycle )
+ {
+ if ( threads < 1 )
+ {
+ throw new IllegalArgumentException( "threads must be greater than zero" );
+ }
+ requireNonNull( lifecycle );
+ this.threads = threads;
+ this.executorService = createExecutorService( threads );
+ lifecycle.addOnSystemEndedHandler( this::shutdown );
+ }
+
+ private ExecutorService createExecutorService( int threads )
+ {
+ if ( threads == 1 )
+ {
+ return null; // we use direct execution
+ }
+ else
+ {
+ return new ThreadPoolExecutor( 0, threads, 3L, TimeUnit.SECONDS,
+ new LinkedBlockingQueue<>(), new WorkerThreadFactory( getClass().getSimpleName() + '-' ) );
+ }
+ }
+
+ @Override
+ public void initService( ServiceLocator locator )
+ {
+ locator.getService( RepositorySystemLifecycle.class ).addOnSystemEndedHandler( this::shutdown );
+ }
+
+ @Override
+ public void submitOrDirect( Collection<Runnable> tasks )
+ {
+ requireNonNull( tasks );
Review Comment:
Bottleneck... or deadlock caused by thread exhaustion.
--
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 #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
gnodet commented on PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#issuecomment-1306971573
> OTOH, I disagree with forkjoinpool: all these threads will do (probably blocking, maybe inifinite) HTTP IO, not to mention the refactoring needed to use them (most of "client" code remained unchanged here, using plain Runnable and Callable).
`ForkJoinPool` is capable of handling blocking operations, just like a standard `ThreadPoolExecutor`. The only problem is when you don't use your own instance and reuse the system provided instance with blocking operations. The only difference between a `ForkJoinPool` and a `ThreadPoolExecutor` is that the former has more features such as thread affinity, work stealing and fork/join operations.
I think a single thread pool is a good idea, but it has to be implemented in a correct way to avoid any thread exhaustion. I.e. use a single fork/join pool and use the appropriate methods to dispatch all the work, which should ensure proper ordering of tasks execution. I've done a similar thing in https://github.com/apache/maven/pull/803 (which I need to finish).
--
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 closed pull request #213: [MRESOLVER-283] Shared executor service
Posted by GitBox <gi...@apache.org>.
cstamas closed pull request #213: [MRESOLVER-283] Shared executor service
URL: https://github.com/apache/maven-resolver/pull/213
--
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 #213: Shared executor
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#issuecomment-1300539170
@caiwei-ebay ping
--
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 #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012585950
##########
src/site/markdown/configuration.md:
##########
@@ -52,7 +52,6 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
`aether.dependencyCollector.maxExceptions` | int | Only exceptions up to the number given in this configuration property are emitted. Exceptions which exceed that number are swallowed. | `50` | no
`aether.dependencyCollector.impl` | String | The name of the dependency collector implementation to use: depth-first (original) named `df`, and breadth-first (new in 1.8.0) named `bf`. Both collectors produce equivalent results, but they may differ performance wise, depending on project being applied to. Our experience shows that existing `df` is well suited for smaller to medium size projects, while `bf` may perform better on huge projects with many dependencies. Experiment (and come back to us!) to figure out which one suits you the better. | `"df"` | no
Review Comment:
I think a dedicated page would be better, instead on this, already very dense configuration page.
--
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 #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012585950
##########
src/site/markdown/configuration.md:
##########
@@ -52,7 +52,6 @@ Option | Type | Description | Default Value | Supports Repo ID Suffix
`aether.dependencyCollector.maxExceptions` | int | Only exceptions up to the number given in this configuration property are emitted. Exceptions which exceed that number are swallowed. | `50` | no
`aether.dependencyCollector.impl` | String | The name of the dependency collector implementation to use: depth-first (original) named `df`, and breadth-first (new in 1.8.0) named `bf`. Both collectors produce equivalent results, but they may differ performance wise, depending on project being applied to. Our experience shows that existing `df` is well suited for smaller to medium size projects, while `bf` may perform better on huge projects with many dependencies. Experiment (and come back to us!) to figure out which one suits you the better. | `"df"` | no
Review Comment:
I think a dedicated page would be better, that on this, already very dense configuration page.
--
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 #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012586206
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/bf/BfDependencyCollector.java:
##########
@@ -475,23 +482,22 @@ else if ( descriptorResult == DataPool.NO_DESCRIPTOR )
static class ParallelDescriptorResolver
{
- final ExecutorService executorService;
+ final ResolverExecutor executor;
/**
* Artifact ID -> Future of DescriptorResolutionResult
*/
final Map<String, Future<DescriptorResolutionResult>> results = new ConcurrentHashMap<>( 256 );
- final Logger logger = LoggerFactory.getLogger( getClass() );
- ParallelDescriptorResolver( RepositorySystemSession session )
+ ParallelDescriptorResolver( ResolverExecutor executor )
{
- this.executorService = getExecutorService( session );
+ this.executor = executor;
}
void resolveDescriptors( Artifact artifact, Callable<DescriptorResolutionResult> callable )
{
results.computeIfAbsent( ArtifactIdUtils.toId( artifact ),
- key -> this.executorService.submit( callable ) );
+ key -> this.executor.submit( callable ) );
Review Comment:
Hopefully I restored the original behaviour, please recheck (latest commit)
--
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 #213: [MRESOLVER-283] Shared executor
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #213:
URL: https://github.com/apache/maven-resolver/pull/213#discussion_r1012585595
##########
maven-resolver-connector-basic/src/main/java/org/eclipse/aether/connector/basic/BasicRepositoryConnector.java:
##########
@@ -656,18 +622,4 @@ private void uploadChecksum( URI location, Object checksum )
}
}
-
- private static class DirectExecutor
- implements Executor
- {
-
- static final Executor INSTANCE = new DirectExecutor();
-
- @Override
- public void execute( Runnable command )
- {
- command.run();
- }
-
Review Comment:
reintroduced direct strategy as well, two out of 3 users of resolver executor now relies on it.
--
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