Changeset 17243 in josm for trunk

Timestamp:

2020-10-19T15:25:47+02:00 (4 years ago)

Author:

GerdP

Message:

see #19885: memory leak with "temporary" objects in validator and actions

• replace ChangeCommand by ChangePropertyCommand to avoid cloning primitives
• also move the call of super.endTest() to the end of the method endTest(), else reported timings make not much sense

Location:

trunk/src/org/openstreetmap/josm/data/validation/tests

Files:

• DuplicateRelation.java (modified) (6 diffs)
• DuplicateWay.java (modified) (2 diffs)
• RelationChecker.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateRelation.java

@@ -14 +14 @@
 import java.util.stream.Collectors;
 
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeMembersCommand;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.DeleteCommand;
@@ -202 +202 @@
     @Override
     public void endTest() {
-        super.endTest();
         for (Set<OsmPrimitive> duplicated : relations.values()) {
             if (duplicated.size() > 1) {
@@ -223 +222 @@
         }
         relationsNoKeys = null;
+        super.endTest();
     }
 
@@ -246 +246 @@
     @Override
     public Command fixError(TestError testError) {
-        if (testError.getCode() == SAME_RELATION) return null;
+        if (!isFixable(testError)) return null;
+
         Set<Relation> relFix = testError.primitives(Relation.class)
-                .filter(r -> !r.isDeleted())
+                .filter(r -> !r.isDeleted() || r.getDataSet() == null || r.getDataSet().getPrimitiveById(r) == null)
                 .collect(Collectors.toSet());
 
@@ -259 +260 @@
         Relation relationWithRelations = null;
         Collection<Relation> relRef = null;
-        for (Relation w : relFix) {
-            Collection<Relation> rel = w.referrers(Relation.class).collect(Collectors.toList());
+        for (Relation r : relFix) {
+            Collection<Relation> rel = r.referrers(Relation.class).collect(Collectors.toList());
             if (!rel.isEmpty()) {
                 if (relationWithRelations != null)
                     throw new AssertionError("Cannot fix duplicate relations: More than one relation is member of another relation.");
-                relationWithRelations = w;
+                relationWithRelations = r;
                 relRef = rel;
             }
             // Only one relation will be kept - the one with lowest positive ID, if such exist
             // or one "at random" if no such exists. Rest of the relations will be deleted
-            if (!w.isNew() && (idToKeep == 0 || w.getId() < idToKeep)) {
-                idToKeep = w.getId();
-                relationToKeep = w;
+            if (!r.isNew() && (idToKeep == 0 || r.getId() < idToKeep)) {
+                idToKeep = r.getId();
+                relationToKeep = r;
             }
         }
@@ -280 +281 @@
         if (relationWithRelations != null && relRef != null && relationToKeep != relationWithRelations) {
             for (Relation rel : relRef) {
-                Relation newRel = new Relation(rel);
-                for (int i = 0; i < newRel.getMembers().size(); ++i) {
-                    RelationMember m = newRel.getMember(i);
+                List<RelationMember> members = new ArrayList<>(rel.getMembers());
+                for (int i = 0; i < rel.getMembers().size(); ++i) {
+                    RelationMember m = rel.getMember(i);
                     if (relationWithRelations.equals(m.getMember())) {
-                        newRel.setMember(i, new RelationMember(m.getRole(), relationToKeep));
+                        members.set(i, new RelationMember(m.getRole(), relationToKeep));
                     }
                 }
-                commands.add(new ChangeCommand(rel, newRel));
+                commands.add(new ChangeMembersCommand(rel, members));
             }
         }

trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateWay.java

@@ -16 +16 @@
 import java.util.stream.IntStream;
 
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeMembersCommand;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.DeleteCommand;
@@ -283 +283 @@
         if (wayWithRelations != null && relations != null && wayToKeep != wayWithRelations) {
             for (Relation rel : relations) {
-                Relation newRel = new Relation(rel);
-                for (int i = 0; i < newRel.getMembers().size(); ++i) {
-                    RelationMember m = newRel.getMember(i);
+                List<RelationMember> members = rel.getMembers();
+                for (int i = 0; i < rel.getMembers().size(); ++i) {
+                    RelationMember m = rel.getMember(i);
                     if (wayWithRelations.equals(m.getMember())) {
-                        newRel.setMember(i, new RelationMember(m.getRole(), wayToKeep));
+                        members.set(i, new RelationMember(m.getRole(), wayToKeep));
                     }
                 }
-                commands.add(new ChangeCommand(rel, newRel));
+                commands.add(new ChangeMembersCommand(rel, members));
             }
         }

trunk/src/org/openstreetmap/josm/data/validation/tests/RelationChecker.java

@@ -18 +18 @@
 import java.util.stream.Collectors;
 
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeMembersCommand;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.DeleteCommand;
@@ -386 +386 @@
             if (testError.getCode() == RELATION_LOOP) {
                 Relation old = (Relation) primitives.iterator().next();
-                Relation mod = new Relation(old);
-                mod.removeMembersFor(primitives);
-                return new ChangeCommand(old, mod);
+                List<RelationMember> remaining = new ArrayList<>(old.getMembers());
+                remaining.removeIf(rm -> primitives.contains(rm.getMember()));
+                return new ChangeMembersCommand(old, Utils.toUnmodifiableList(remaining));
             }
         }