From 9e00328d330a6208e996135d23f6d54be0c0de30 Mon Sep 17 00:00:00 2001 From: Taylor Smock Date: Wed, 1 Jun 2022 16:47:04 -0600 Subject: [PATCH] Decrease allocations in various locations With the changes from 325c169468bce987bd93e78da81b92c12e52d86f..2c11de9e3ca95e5ea26ba08b053692ff013eb728, this reduces allocations due to MapWithAI cleanup methods (on download) from 1.7 GB to 265 MB. Signed-off-by: Taylor Smock --- .../actions/AddMapWithAILayerAction.java | 26 ++-- .../BoundingBoxMapWithAIDownloader.java | 5 +- .../backend/DownloadMapWithAITask.java | 21 ++- .../mapwithai/backend/GetDataRunnable.java | 137 ++++++++++++------ .../mapwithai/backend/MapWithAIDataUtils.java | 5 +- 5 files changed, 119 insertions(+), 75 deletions(-) diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/actions/AddMapWithAILayerAction.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/actions/AddMapWithAILayerAction.java index db0a440..d71562e 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/actions/AddMapWithAILayerAction.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/actions/AddMapWithAILayerAction.java @@ -4,12 +4,17 @@ package org.openstreetmap.josm.plugins.mapwithai.actions; import static org.openstreetmap.josm.gui.help.HelpUtil.ht; import java.awt.event.ActionEvent; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.ExecutionException; +import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.ForkJoinTask; import java.util.concurrent.Future; import org.openstreetmap.josm.actions.AdaptableAction; import org.openstreetmap.josm.actions.AddImageryLayerAction; import org.openstreetmap.josm.actions.JosmAction; +import org.openstreetmap.josm.data.Bounds; import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.OsmData; import org.openstreetmap.josm.gui.MainApplication; @@ -17,8 +22,6 @@ import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.gui.preferences.ToolbarPreferences; import org.openstreetmap.josm.gui.progress.NullProgressMonitor; import org.openstreetmap.josm.gui.util.GuiHelper; -import org.openstreetmap.josm.io.OsmTransferException; -import org.openstreetmap.josm.plugins.mapwithai.backend.BoundingBoxMapWithAIDownloader; import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAIDataUtils; import org.openstreetmap.josm.plugins.mapwithai.backend.MapWithAILayer; import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIInfo; @@ -101,14 +104,17 @@ public class AddMapWithAILayerAction extends JosmAction implements AdaptableActi ds = null; } if (boundsSource != null && ds != null) { - boundsSource.getDataSourceBounds().forEach(b -> MainApplication.worker.execute(() -> { - try { - ds.mergeFrom( - new BoundingBoxMapWithAIDownloader(b, info, false).parseOsm(NullProgressMonitor.INSTANCE)); - } catch (OsmTransferException error) { - Logging.error(error); - } - })); + ForkJoinPool pool = MapWithAIDataUtils.getForkJoinPool(); + List> forkJoinTasks = new ArrayList<>(boundsSource.getDataSourceBounds().size()); + for (Bounds b : boundsSource.getDataSourceBounds()) { + ForkJoinTask task = MapWithAIDataUtils.download(NullProgressMonitor.INSTANCE, b, info, + MapWithAIDataUtils.MAXIMUM_SIDE_DIMENSIONS); + forkJoinTasks.add(task); + pool.execute(task); + } + for (ForkJoinTask task : forkJoinTasks) { + ds.mergeFrom(task.join()); + } } if (layer != null) { layer.addDownloadedInfo(info); diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/BoundingBoxMapWithAIDownloader.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/BoundingBoxMapWithAIDownloader.java index 0822843..73c56e4 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/BoundingBoxMapWithAIDownloader.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/BoundingBoxMapWithAIDownloader.java @@ -83,10 +83,7 @@ public class BoundingBoxMapWithAIDownloader extends BoundingBoxDownloader { public DataSet parseOsm(ProgressMonitor progressMonitor) throws OsmTransferException { long startTime = System.nanoTime(); try { - DataSet externalData; - synchronized (BoundingBoxMapWithAIDownloader.class) { - externalData = super.parseOsm(progressMonitor); - } + DataSet externalData = super.parseOsm(progressMonitor); if (Boolean.TRUE.equals(MapWithAIInfo.THIRD_PARTY_CONFLATE.get()) && !this.info.isConflated() && !MapWithAIConflationCategory.conflationUrlFor(this.info.getCategory()).isEmpty()) { if (externalData.getDataSourceBounds().isEmpty()) { diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/DownloadMapWithAITask.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/DownloadMapWithAITask.java index 1b92055..5e974eb 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/DownloadMapWithAITask.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/DownloadMapWithAITask.java @@ -9,6 +9,8 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.ForkJoinTask; import java.util.concurrent.Future; import java.util.stream.Collectors; @@ -18,7 +20,6 @@ import org.openstreetmap.josm.data.Bounds; import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.Notification; -import org.openstreetmap.josm.gui.progress.NullProgressMonitor; import org.openstreetmap.josm.gui.progress.ProgressMonitor; import org.openstreetmap.josm.gui.util.GuiHelper; import org.openstreetmap.josm.io.OsmServerReader; @@ -99,15 +100,10 @@ public class DownloadMapWithAITask extends DownloadOsmTask { } class DownloadTask extends AbstractInternalTask { - BoundingBoxMapWithAIDownloader downloader; + List> downloader; final Bounds bounds; private List relevantUrls; - DownloadTask(DownloadParams settings, String title, boolean ignoreException, boolean zoomAfterDownload, - Bounds bounds) { - this(settings, title, NullProgressMonitor.INSTANCE, ignoreException, zoomAfterDownload, bounds); - } - public DownloadTask(DownloadParams settings, String title, ProgressMonitor progressMonitor, boolean ignoreException, boolean zoomAfterDownload, Bounds bounds) { super(settings, title, progressMonitor, ignoreException, zoomAfterDownload); @@ -118,7 +114,7 @@ public class DownloadMapWithAITask extends DownloadOsmTask { protected void cancel() { setCanceled(true); if (downloader != null) { - downloader.cancel(); + downloader.forEach(task -> task.cancel(true)); } } @@ -137,14 +133,17 @@ public class DownloadMapWithAITask extends DownloadOsmTask { monitor.setTicksCount(relevantUrls.size()); } downloadedData = new DataSet(); + final ForkJoinPool pool = MapWithAIDataUtils.getForkJoinPool(); + this.downloader = new ArrayList<>(relevantUrls.size()); for (MapWithAIInfo info : relevantUrls) { if (isCanceled()) { break; } - downloader = new BoundingBoxMapWithAIDownloader(bounds, info, false); - DataSet ds = downloader.parseOsm(monitor.createSubTaskMonitor(1, true)); - downloadedData.mergeFrom(ds); + this.downloader.add(pool.submit(MapWithAIDataUtils.download(this.progressMonitor, bounds, info, + MapWithAIDataUtils.MAXIMUM_SIDE_DIMENSIONS))); } + this.downloader + .forEach(task -> downloadedData.mergeFrom(task.join(), monitor.createSubTaskMonitor(1, false))); } @Override diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/GetDataRunnable.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/GetDataRunnable.java index d5bb68a..f846908 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/GetDataRunnable.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/GetDataRunnable.java @@ -4,6 +4,7 @@ package org.openstreetmap.josm.plugins.mapwithai.backend; import static org.openstreetmap.josm.tools.I18n.tr; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.HashSet; @@ -11,8 +12,12 @@ import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.TreeMap; +import java.util.concurrent.ForkJoinPool; +import java.util.concurrent.ForkJoinTask; import java.util.concurrent.RecursiveTask; +import java.util.function.BiPredicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -28,6 +33,8 @@ import org.openstreetmap.josm.data.osm.DataSet; import org.openstreetmap.josm.data.osm.Hash; import org.openstreetmap.josm.data.osm.INode; import org.openstreetmap.josm.data.osm.IPrimitive; +import org.openstreetmap.josm.data.osm.IRelation; +import org.openstreetmap.josm.data.osm.IWay; import org.openstreetmap.josm.data.osm.IWaySegment; import org.openstreetmap.josm.data.osm.Node; import org.openstreetmap.josm.data.osm.OsmPrimitive; @@ -37,12 +44,12 @@ import org.openstreetmap.josm.data.osm.Tag; import org.openstreetmap.josm.data.osm.TagMap; import org.openstreetmap.josm.data.osm.UploadPolicy; import org.openstreetmap.josm.data.osm.Way; +import org.openstreetmap.josm.data.osm.visitor.PrimitiveVisitor; import org.openstreetmap.josm.data.projection.ProjectionRegistry; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.layer.OsmDataLayer; import org.openstreetmap.josm.gui.progress.NullProgressMonitor; import org.openstreetmap.josm.gui.progress.ProgressMonitor; -import org.openstreetmap.josm.io.OsmTransferException; import org.openstreetmap.josm.plugins.mapwithai.MapWithAIPlugin; import org.openstreetmap.josm.plugins.mapwithai.commands.MergeDuplicateWays; import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAIInfo; @@ -50,7 +57,6 @@ import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.MapWithAILayerInf import org.openstreetmap.josm.plugins.mapwithai.data.mapwithai.PreConflatedDataUtils; import org.openstreetmap.josm.tools.Geometry; import org.openstreetmap.josm.tools.JosmRuntimeException; -import org.openstreetmap.josm.tools.Logging; import org.openstreetmap.josm.tools.Pair; import org.openstreetmap.josm.tools.Utils; @@ -111,6 +117,30 @@ public class GetDataRunnable extends RecursiveTask { } } + /** + * This checks that all visited objects are highways + */ + private static final class HighwayVisitor implements PrimitiveVisitor { + boolean onlyHighways = true; + + @Override + public void visit(INode n) { + onlyHighways = false; + } + + @Override + public void visit(IWay w) { + if (!w.isTagged() || !w.hasTag("highway")) { + onlyHighways = false; + } + } + + @Override + public void visit(IRelation r) { + onlyHighways = false; + } + } + private static final long serialVersionUID = 258423685658089715L; private final transient List runnableBounds; private final transient DataSet dataSet; @@ -416,11 +446,22 @@ public class GetDataRunnable extends RecursiveTask { * @param dataSet The dataset to remove tags from */ public static void removeCommonTags(DataSet dataSet) { - dataSet.allPrimitives().stream().filter(prim -> prim.hasKey(MergeDuplicateWays.ORIG_ID)) - .forEach(prim -> prim.remove(MergeDuplicateWays.ORIG_ID)); - dataSet.getNodes().forEach(node -> node.remove(SERVER_ID_KEY)); - final List emptyNodes = dataSet.getNodes().stream().distinct().filter(node -> !node.isDeleted()) - .filter(node -> node.getReferrers().isEmpty() && !node.hasKeys()).collect(Collectors.toList()); + final Set emptyNodes = new HashSet<>(); + for (OsmPrimitive tagged : dataSet.allPrimitives()) { + if (!tagged.hasKeys()) { + continue; + } + if (tagged.hasKey(MergeDuplicateWays.ORIG_ID)) { + tagged.remove(MergeDuplicateWays.ORIG_ID); + } + if (tagged instanceof Node) { + tagged.remove(SERVER_ID_KEY); + final Node node = (Node) tagged; + if (!node.isDeleted() && !node.hasKeys() && node.getReferrers().isEmpty()) { + emptyNodes.add(node); + } + } + } if (!emptyNodes.isEmpty()) { new DeleteCommand(emptyNodes).executeCommand(); } @@ -483,21 +524,22 @@ public class GetDataRunnable extends RecursiveTask { return basicNodeChecks(nearNode, node) && onlyHasHighwayParents(node) && ((keyCheck(nearNode, node) && distanceCheck(nearNode, node, MapWithAIPreferenceHelper.getMaxNodeDistance())) - || (!nearNode.getKeys().isEmpty() && nearNode.getKeys().equals(node.getKeys()) + || (nearNode.hasKeys() && node.hasKeys() && nearNode.getKeys().equals(node.getKeys()) && distanceCheck(nearNode, node, MapWithAIPreferenceHelper.getMaxNodeDistance() * 10))); } - private static boolean distanceCheck(INode nearNode, INode node, Double distance) { - return !(nearNode == null || node == null || nearNode.getCoor() == null || node.getCoor() == null) - && nearNode.getCoor().greatCircleDistance(node.getCoor()) < distance; + private static boolean distanceCheck(Node nearNode, Node node, Double distance) { + return Geometry.getDistance(nearNode, node) < distance; } private static boolean keyCheck(INode nearNode, INode node) { - return nearNode.getKeys().equals(node.getKeys()) || nearNode.getKeys().isEmpty() || node.getKeys().isEmpty(); + return !nearNode.hasKeys() || !node.hasKeys() || nearNode.getKeys().equals(node.getKeys()); } private static boolean onlyHasHighwayParents(Node node) { - return node.referrers(OsmPrimitive.class).allMatch(prim -> prim.hasKey("highway")); + HighwayVisitor highwayVisitor = new HighwayVisitor(); + node.visitReferrers(highwayVisitor); + return highwayVisitor.onlyHighways; } private static boolean basicNodeChecks(INode nearNode, INode node) { @@ -506,16 +548,21 @@ public class GetDataRunnable extends RecursiveTask { } private static void mergeWays(DataSet dataSet) { - final List ways = dataSet.getWays().stream().filter(way -> !way.isDeleted()).collect(Collectors.toList()); - for (final Way way1 : ways) { + for (final Way way1 : dataSet.getWays()) { + if (way1.isDeleted()) { + continue; + } final BBox bbox = new BBox(); bbox.addPrimitive(way1, DEGREE_BUFFER); - final List nearbyWays = dataSet.searchWays(bbox).stream() - .filter(way -> way.getNodes().stream().filter(node -> way1.getNodes().contains(node)).count() > 1) - .collect(Collectors.toList()); - way1.getNodePairs(false); - nearbyWays.stream().flatMap(way2 -> checkWayDuplications(way1, way2).entrySet().stream()) - .forEach(GetDataRunnable::addMissingElement); + for (Way nearbyWay : dataSet.searchWays(bbox)) { + if (nearbyWay.getNodes().stream().filter(way1::containsNode).count() > 1) { + // way1.getNodePairs(false); + for (Map.Entry, List>> entry : checkWayDuplications( + way1, nearbyWay).entrySet()) { + GetDataRunnable.addMissingElement(entry); + } + } + } } } @@ -589,32 +636,26 @@ public class GetDataRunnable extends RecursiveTask { final List> waySegments2 = way2.getNodePairs(false).stream() .map(pair -> IWaySegment.forNodePair(way2, pair.a, pair.b)).collect(Collectors.toList()); final Map, List>> partials = new TreeMap<>(); + final BiPredicate, IWaySegment> connected = (segment1, + segment2) -> segment1.getFirstNode().equals(segment2.getFirstNode()) + || segment1.getSecondNode().equals(segment2.getFirstNode()) + || segment1.getFirstNode().equals(segment2.getSecondNode()) + || segment1.getSecondNode().equals(segment2.getSecondNode()); for (final IWaySegment segment1 : waySegments1) { - final Way waySegment1; - try { - waySegment1 = segment1.toWay(); - } catch (ReflectiveOperationException e) { - throw new JosmRuntimeException(e); - } final List> replacements = waySegments2.stream() - .filter(seg2 -> waySegment1.isFirstLastNode(seg2.getFirstNode()) - || waySegment1.isFirstLastNode(seg2.getSecondNode())) - .filter(seg -> { - final Node node2 = waySegment1.isFirstLastNode(seg.getFirstNode()) ? seg.getFirstNode() - : seg.getSecondNode(); + .filter(seg2 -> connected.test(segment1, seg2)).filter(seg -> { + final Node node2 = segment1.getFirstNode().equals(seg.getFirstNode()) + || segment1.getSecondNode().equals(seg.getFirstNode()) ? seg.getFirstNode() + : seg.getSecondNode(); final Node node1 = node2.equals(seg.getFirstNode()) ? seg.getSecondNode() : seg.getFirstNode(); - final Node node3 = waySegment1.getNode(0).equals(node2) ? waySegment1.getNode(1) - : waySegment1.getNode(0); + final Node node3 = segment1.getFirstNode().equals(node2) ? segment1.getSecondNode() + : segment1.getFirstNode(); return Math.abs(Geometry.getCornerAngle(node1.getEastNorth(), node2.getEastNorth(), node3.getEastNorth())) < (Math.PI / 4); }).collect(Collectors.toList()); - if ((replacements.size() != 2) || replacements.stream().anyMatch(seg -> { - try { - return new HashSet<>(waySegment1.getNodes()).containsAll(seg.toWay().getNodes()); - } catch (ReflectiveOperationException e) { - throw new JosmRuntimeException(e); - } - })) { + if ((replacements.size() != 2) || replacements.stream() + .anyMatch(seg -> Arrays.asList(segment1.getFirstNode(), segment1.getSecondNode()) + .containsAll(Arrays.asList(seg.getFirstNode(), seg.getSecondNode())))) { continue; } partials.put(segment1, replacements); @@ -633,14 +674,14 @@ public class GetDataRunnable extends RecursiveTask { final DataSet dataSet = new DataSet(); dataSet.setUploadPolicy(UploadPolicy.DISCOURAGED); + final List> tasks = new ArrayList<>(); + final ForkJoinPool pool = MapWithAIDataUtils.getForkJoinPool(); for (MapWithAIInfo map : new ArrayList<>(MapWithAILayerInfo.getInstance().getLayers())) { - try { - BoundingBoxMapWithAIDownloader downloader = new BoundingBoxMapWithAIDownloader(bounds, map, - DetectTaskingManagerUtils.hasTaskingManagerLayer()); - dataSet.mergeFrom(downloader.parseOsm(monitor)); - } catch (OsmTransferException e1) { - Logging.debug(e1); - } + tasks.add(pool.submit( + MapWithAIDataUtils.download(monitor, bounds, map, MapWithAIDataUtils.MAXIMUM_SIDE_DIMENSIONS))); + } + for (ForkJoinTask task : tasks) { + dataSet.mergeFrom(task.join()); } dataSet.setUploadPolicy(UploadPolicy.BLOCKED); return dataSet; diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java index 58d9ea3..0da0111 100644 --- a/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java +++ b/src/main/java/org/openstreetmap/josm/plugins/mapwithai/backend/MapWithAIDataUtils.java @@ -33,6 +33,7 @@ import org.openstreetmap.josm.gui.ConditionalOptionPaneUtil; import org.openstreetmap.josm.gui.MainApplication; import org.openstreetmap.josm.gui.Notification; import org.openstreetmap.josm.gui.layer.OsmDataLayer; +import org.openstreetmap.josm.gui.progress.ProgressMonitor; import org.openstreetmap.josm.gui.progress.swing.PleaseWaitProgressMonitor; import org.openstreetmap.josm.gui.util.GuiHelper; import org.openstreetmap.josm.io.IllegalDataException; @@ -160,8 +161,8 @@ public final class MapWithAIDataUtils { * @param maximumDimensions The maximum dimensions to download * @return A future that will have downloaded the data */ - private static ForkJoinTask download(PleaseWaitProgressMonitor monitor, Bounds bound, - MapWithAIInfo mapWithAIInfo, int maximumDimensions) { + public static ForkJoinTask download(ProgressMonitor monitor, Bounds bound, MapWithAIInfo mapWithAIInfo, + int maximumDimensions) { return ForkJoinTask.adapt(() -> { BoundingBoxMapWithAIDownloader downloader = new BoundingBoxMapWithAIDownloader(bound, mapWithAIInfo, DetectTaskingManagerUtils.hasTaskingManagerLayer());