From c5ca425e69cec4a097dce48f3d2f27e0f534f515 Mon Sep 17 00:00:00 2001 From: Hannes Wellmann Date: Sat, 2 Sep 2023 14:20:08 +0200 Subject: [PATCH] Enhance ProgressMonitor usage in ProvisioningContext - Use SubMonitor.split() over newChild() - Remove explicit checks for cancellation since SubMonitor.split() does that implicitly - Simplify work computation by avoiding unnecessary multiplies When creating a split SubMonitor the splitted child only operates on the assigned amount of work. --- .../META-INF/MANIFEST.MF | 2 +- .../p2/engine/ProvisioningContext.java | 93 ++++++++----------- 2 files changed, 40 insertions(+), 55 deletions(-) diff --git a/bundles/org.eclipse.equinox.p2.engine/META-INF/MANIFEST.MF b/bundles/org.eclipse.equinox.p2.engine/META-INF/MANIFEST.MF index 1a64af4a08..a87404a127 100644 --- a/bundles/org.eclipse.equinox.p2.engine/META-INF/MANIFEST.MF +++ b/bundles/org.eclipse.equinox.p2.engine/META-INF/MANIFEST.MF @@ -2,7 +2,7 @@ Manifest-Version: 1.0 Bundle-ManifestVersion: 2 Bundle-Name: %pluginName Bundle-SymbolicName: org.eclipse.equinox.p2.engine;singleton:=true -Bundle-Version: 2.8.100.qualifier +Bundle-Version: 2.8.200.qualifier Bundle-Activator: org.eclipse.equinox.internal.p2.engine.EngineActivator Bundle-Vendor: %providerName Bundle-Localization: plugin diff --git a/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/p2/engine/ProvisioningContext.java b/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/p2/engine/ProvisioningContext.java index 5010f166fa..30629c8065 100644 --- a/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/p2/engine/ProvisioningContext.java +++ b/bundles/org.eclipse.equinox.p2.engine/src/org/eclipse/equinox/p2/engine/ProvisioningContext.java @@ -40,7 +40,7 @@ public class ProvisioningContext { private IProvisioningAgent agent; private URI[] artifactRepositories; //artifact repositories to consult - private final List extraIUs = Collections.synchronizedList(new ArrayList()); + private final List extraIUs = Collections.synchronizedList(new ArrayList<>()); private URI[] metadataRepositories; //metadata repositories to consult private final Map properties = new HashMap<>(); private Map referencedArtifactRepositories = null; @@ -171,9 +171,9 @@ private List getLoadedArtifactRepositories(IProgressMonitor Arrays.sort(repositories, LOCAL_FIRST_COMPARATOR); List repos = new ArrayList<>(); - SubMonitor sub = SubMonitor.convert(monitor, (repositories.length + 1) * 100); + SubMonitor subMonitor = SubMonitor.convert(monitor, repositories.length + 1); for (URI location : repositories) { - getLoadedRepository(location, repoManager, repos, sub); + getLoadedRepository(location, repoManager, repos, subMonitor.split(1)); // Remove this URI from the list of extra references if it is there. if (referencedArtifactRepositories != null && location != null) { referencedArtifactRepositories.remove(location.toString()); @@ -181,25 +181,23 @@ private List getLoadedArtifactRepositories(IProgressMonitor } // Are there any extra artifact repository references to consider? if (referencedArtifactRepositories != null && referencedArtifactRepositories.size() > 0 && shouldFollowArtifactReferences()) { - SubMonitor innerSub = SubMonitor.convert(sub.newChild(100), referencedArtifactRepositories.size() * 100); + subMonitor.setWorkRemaining(referencedArtifactRepositories.size()); for (URI referencedURI : referencedArtifactRepositories.values()) { - getLoadedRepository(referencedURI, repoManager, repos, innerSub); + getLoadedRepository(referencedURI, repoManager, repos, subMonitor.split(1)); } } return repos; } - private void getLoadedRepository(URI location, IArtifactRepositoryManager repoManager, List repos, SubMonitor monitor) { - if (monitor.isCanceled()) { - throw new OperationCanceledException(); - } + private void getLoadedRepository(URI location, IArtifactRepositoryManager repoManager, + List repos, IProgressMonitor monitor) { if (failedArtifactRepositories.contains(location)) { return; } try { IArtifactRepository repository = loadedArtifactRepositories.get(location); if (repository == null) { - repository = repoManager.loadRepository(location, monitor.newChild(100)); + repository = repoManager.loadRepository(location, monitor); loadedArtifactRepositories.put(location, repository); } repos.add(repository); @@ -213,37 +211,33 @@ private Set getLoadedMetadataRepositories(IProgressMonitor IMetadataRepositoryManager repoManager = agent.getService(IMetadataRepositoryManager.class); URI[] repositories = metadataRepositories == null ? repoManager.getKnownRepositories(IRepositoryManager.REPOSITORIES_ALL) : metadataRepositories; - HashMap repos = new HashMap<>(); - SubMonitor sub = SubMonitor.convert(monitor, repositories.length * 100); + Map repos = new HashMap<>(); + SubMonitor sub = SubMonitor.convert(monitor, repositories.length); // Clear out the list of remembered artifact repositories referencedArtifactRepositories = new HashMap<>(); for (URI repositorie : repositories) { - if (sub.isCanceled()) - throw new OperationCanceledException(); - loadMetadataRepository(repoManager, repositorie, repos, shouldFollowReferences(), sub.newChild(100)); + loadMetadataRepository(repoManager, repositorie, repos, shouldFollowReferences(), sub.split(1)); } Set set = new HashSet<>(); set.addAll(repos.values()); return set; } - private void loadMetadataRepository(IMetadataRepositoryManager manager, URI location, HashMap repos, boolean followMetadataRepoReferences, IProgressMonitor monitor) { - if (monitor.isCanceled()) { - throw new OperationCanceledException(); - } + private void loadMetadataRepository(IMetadataRepositoryManager manager, URI location, + Map repos, boolean followMetadataRepoReferences, IProgressMonitor monitor) { + SubMonitor subMonitor = SubMonitor.convert(monitor, 2); // if we've already processed this repo, don't do it again. This keeps us from getting // caught up in circular references. if (repos.containsKey(location.toString()) || failedMetadataRepositories.contains(location)) { return; } - SubMonitor sub = SubMonitor.convert(monitor, 1000); // First load the repository itself. IMetadataRepository repository = loadedMetadataRepositories.get(location); if (repository == null) { try { - repository = manager.loadRepository(location, sub.newChild(500)); + repository = manager.loadRepository(location, subMonitor.split(1)); loadedMetadataRepositories.put(location, repository); } catch (ProvisionException e) { failedMetadataRepositories.add(location); @@ -254,25 +248,22 @@ private void loadMetadataRepository(IMetadataRepositoryManager manager, URI loca Collection references = repository.getReferences(); // We always load artifact repositories referenced by this repository. We might load // metadata repositories - if (references.size() > 0) { + if (!references.isEmpty()) { IArtifactRepositoryManager artifactManager = agent.getService(IArtifactRepositoryManager.class); - SubMonitor repoSubMon = SubMonitor.convert(sub.newChild(500), 100 * references.size()); + subMonitor.setWorkRemaining(references.size()); for (IRepositoryReference ref : references) { try { if (ref.getType() == IRepository.TYPE_METADATA && followMetadataRepoReferences && isEnabled(manager, ref)) { - loadMetadataRepository(manager, ref.getLocation(), repos, followMetadataRepoReferences, repoSubMon.newChild(100)); - } else if (ref.getType() == IRepository.TYPE_ARTIFACT) { + loadMetadataRepository(manager, ref.getLocation(), repos, followMetadataRepoReferences, subMonitor.split(1)); + } else if (ref.getType() == IRepository.TYPE_ARTIFACT && isEnabled(artifactManager, ref)) { // We want to remember all enabled artifact repository locations. - if (isEnabled(artifactManager, ref)) - referencedArtifactRepositories.put(ref.getLocation().toString(), ref.getLocation()); + referencedArtifactRepositories.put(ref.getLocation().toString(), ref.getLocation()); } } catch (IllegalArgumentException e) { // see https://bugs.eclipse.org/bugs/show_bug.cgi?id=311338 // ignore invalid location and keep going } } - } else { - sub.done(); } } @@ -356,20 +347,22 @@ private interface Manager { private Collection getAllLoadedMetadataRepositories(IProgressMonitor monitor) { if (allLoadedMetadataRepositories == null) { + SubMonitor subMonitor = SubMonitor.convert(monitor, 2); var repoManager = agent.getService(IMetadataRepositoryManager.class); - getLoadedMetadataRepositories(monitor); + getLoadedMetadataRepositories(subMonitor.split(1)); allLoadedMetadataRepositories = getAllLoadedRepositories(repoManager::loadRepository, - loadedMetadataRepositories, failedMetadataRepositories, monitor); + loadedMetadataRepositories, failedMetadataRepositories, subMonitor.split(1)); } return allLoadedMetadataRepositories.values(); } private Collection getAllLoadedArtifactRepositories(IProgressMonitor monitor) { if (allLoadedArtifactRepositories == null) { + SubMonitor subMonitor = SubMonitor.convert(monitor, 2); var repoManager = agent.getService(IArtifactRepositoryManager.class); - getLoadedArtifactRepositories(monitor); + getLoadedArtifactRepositories(subMonitor.split(1)); allLoadedArtifactRepositories = getAllLoadedRepositories(repoManager::loadRepository, - loadedArtifactRepositories, failedArtifactRepositories, monitor); + loadedArtifactRepositories, failedArtifactRepositories, subMonitor.split(1)); } return allLoadedArtifactRepositories.values(); } @@ -378,14 +371,10 @@ private Map getAllLoadedRepositories(Manager manager, Map loadedRepositories, Set failedRepositories, IProgressMonitor monitor) { + SubMonitor subMonitor = SubMonitor.convert(monitor, loadedRepositories.size()); var allLoadedRepositories = new HashMap<>(loadedRepositories); - if (!loadedRepositories.isEmpty()) { - for (var repository : loadedRepositories.values()) { - loadComposites(manager, repository, allLoadedRepositories, failedRepositories, monitor); - if (monitor.isCanceled()) { - throw new OperationCanceledException(); - } - } + for (var repository : loadedRepositories.values()) { + loadComposites(manager, repository, allLoadedRepositories, failedRepositories, subMonitor.split(1)); } return allLoadedRepositories; } @@ -393,17 +382,17 @@ private Map getAllLoadedRepositories(Manager manager, private void loadComposites(Manager manager, T repository, Map repos, Set failedRepositories, IProgressMonitor monitor) { if (repository instanceof ICompositeRepository composite) { - for (var location : composite.getChildren()) { - loadRepository(manager, location, repos, failedRepositories, monitor); + List children = composite.getChildren(); + SubMonitor subMonitor = SubMonitor.convert(monitor, children.size()); + for (var location : children) { + loadRepository(manager, location, repos, failedRepositories, subMonitor.split(1)); } } } private void loadRepository(Manager manager, URI location, Map repos, Set failedRepositories, IProgressMonitor monitor) { - if (monitor.isCanceled()) { - throw new OperationCanceledException(); - } + SubMonitor subMonitor = SubMonitor.convert(monitor, 2); if (repos.containsKey(location) || failedMetadataRepositories.contains(location)) { return; @@ -412,22 +401,18 @@ private void loadRepository(Manager manager, URI location, Map re var repository = repos.get(location); if (repository == null) { try { - repository = manager.loadRepository(location, monitor); + repository = manager.loadRepository(location, subMonitor.split(1)); repos.put(location, repository); - loadComposites(manager, repository, repos, failedRepositories, monitor); + loadComposites(manager, repository, repos, failedRepositories, subMonitor.split(1)); } catch (ProvisionException e) { failedMetadataRepositories.add(location); - return; } } } - private static Comparator ARTIFACT_KEY_COMPARATOR = (o1, o2) -> { - var cmp = o1.getId().compareTo(o2.getId()); - if (cmp == 0) - cmp = o1.getVersion().compareTo(o2.getVersion()); - return cmp; - }; + private static final Comparator ARTIFACT_KEY_COMPARATOR = Comparator // + .comparing(IArtifactKey::getId) // + .thenComparing(IArtifactKey::getVersion); /** * Returns a map from simple artifact repository location to a subset of the