Index: trunk/src/org/openstreetmap/josm/data/osm/DataSet.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/DataSet.java (revision 3347) +++ trunk/src/org/openstreetmap/josm/data/osm/DataSet.java (revision 3348) @@ -15,4 +15,7 @@ import java.util.List; import java.util.Map; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; import org.openstreetmap.josm.data.SelectionChangedListener; @@ -42,4 +45,14 @@ public class DataSet implements Cloneable { + /** + * Maximum number of events that can be fired between beginUpdate/endUpdate to be send as single events (ie without DatasetChangedEvent) + */ + private static final int MAX_SINGLE_EVENTS = 30; + + /** + * Maximum number of events to kept between beginUpdate/endUpdate. When more events are created, that simple DatasetChangedEvent is sent) + */ + private static final int MAX_EVENTS = 1000; + private static class IdHash implements Hash { @@ -57,8 +70,17 @@ private Map primitivesMap = allPrimitives.foreignKey(new IdHash()); private List listeners = new ArrayList(); + // Number of open calls to beginUpdate private int updateCount; + // Events that occurred while dataset was locked but should be fired after write lock is released + private final List cachedEvents = new ArrayList(); private int highlightUpdateCount; + + private final ReadWriteLock lock = new ReentrantReadWriteLock(); + + public Lock getReadLock() { + return lock.readLock(); + } /** @@ -212,35 +234,45 @@ */ public void addPrimitive(OsmPrimitive primitive) { - if (getPrimitiveById(primitive) != null) - throw new DataIntegrityProblemException( - tr("Unable to add primitive {0} to the dataset because it is already included", primitive.toString())); - - primitive.updatePosition(); // Set cached bbox for way and relation (required for reindexWay and reinexRelation to work properly) - if (primitive instanceof Node) { - nodes.add((Node) primitive); - } else if (primitive instanceof Way) { - ways.add((Way) primitive); - } else if (primitive instanceof Relation) { - relations.add((Relation) primitive); - } - allPrimitives.add(primitive); - primitive.setDataset(this); - firePrimitivesAdded(Collections.singletonList(primitive), false); + beginUpdate(); + try { + if (getPrimitiveById(primitive) != null) + throw new DataIntegrityProblemException( + tr("Unable to add primitive {0} to the dataset because it is already included", primitive.toString())); + + primitive.updatePosition(); // Set cached bbox for way and relation (required for reindexWay and reinexRelation to work properly) + if (primitive instanceof Node) { + nodes.add((Node) primitive); + } else if (primitive instanceof Way) { + ways.add((Way) primitive); + } else if (primitive instanceof Relation) { + relations.add((Relation) primitive); + } + allPrimitives.add(primitive); + primitive.setDataset(this); + firePrimitivesAdded(Collections.singletonList(primitive), false); + } finally { + endUpdate(); + } } public OsmPrimitive addPrimitive(PrimitiveData data) { - OsmPrimitive result; - if (data instanceof NodeData) { - result = new Node(); - } else if (data instanceof WayData) { - result = new Way(); - } else if (data instanceof RelationData) { - result = new Relation(); - } else - throw new AssertionError(); - result.setDataset(this); - result.load(data); - addPrimitive(result); - return result; + beginUpdate(); + try { + OsmPrimitive result; + if (data instanceof NodeData) { + result = new Node(); + } else if (data instanceof WayData) { + result = new Way(); + } else if (data instanceof RelationData) { + result = new Relation(); + } else + throw new AssertionError(); + result.setDataset(this); + result.load(data); + addPrimitive(result); + return result; + } finally { + endUpdate(); + } } @@ -255,18 +287,23 @@ */ public void removePrimitive(PrimitiveId primitiveId) { - OsmPrimitive primitive = getPrimitiveByIdChecked(primitiveId); - if (primitive == null) - return; - if (primitive instanceof Node) { - nodes.remove(primitive); - } else if (primitive instanceof Way) { - ways.remove(primitive); - } else if (primitive instanceof Relation) { - relations.remove(primitive); - } - selectedPrimitives.remove(primitive); - allPrimitives.remove(primitive); - primitive.setDataset(null); - firePrimitivesRemoved(Collections.singletonList(primitive), false); + beginUpdate(); + try { + OsmPrimitive primitive = getPrimitiveByIdChecked(primitiveId); + if (primitive == null) + return; + if (primitive instanceof Node) { + nodes.remove(primitive); + } else if (primitive instanceof Way) { + ways.remove(primitive); + } else if (primitive instanceof Relation) { + relations.remove(primitive); + } + selectedPrimitives.remove(primitive); + allPrimitives.remove(primitive); + primitive.setDataset(null); + firePrimitivesRemoved(Collections.singletonList(primitive), false); + } finally { + endUpdate(); + } } @@ -492,110 +529,115 @@ */ -// public void setDisabled(OsmPrimitive... osm) { -// if (osm.length == 1 && osm[0] == null) { -// setDisabled(); -// return; -// } -// clearDisabled(allPrimitives()); -// for (OsmPrimitive o : osm) -// if (o != null) { -// o.setDisabled(true); -// } -// } -// -// public void setDisabled(Collection selection) { -// clearDisabled(nodes); -// clearDisabled(ways); -// clearDisabled(relations); -// for (OsmPrimitive osm : selection) { -// osm.setDisabled(true); -// } -// } -// -// /** -// * Remove the disabled parameter from every value in the collection. -// * @param list The collection to remove the disabled parameter from. -// */ -// private void clearDisabled(Collection list) { -// for (OsmPrimitive osm : list) { -// osm.setDisabled(false); -// } -// } -// -// -// public void setFiltered(Collection selection) { -// clearFiltered(nodes); -// clearFiltered(ways); -// clearFiltered(relations); -// for (OsmPrimitive osm : selection) { -// osm.setFiltered(true); -// } -// } -// -// public void setFiltered(OsmPrimitive... osm) { -// if (osm.length == 1 && osm[0] == null) { -// setFiltered(); -// return; -// } -// clearFiltered(nodes); -// clearFiltered(ways); -// clearFiltered(relations); -// for (OsmPrimitive o : osm) -// if (o != null) { -// o.setFiltered(true); -// } -// } -// -// /** -// * Remove the filtered parameter from every value in the collection. -// * @param list The collection to remove the filtered parameter from. -// */ -// private void clearFiltered(Collection list) { -// if (list == null) -// return; -// for (OsmPrimitive osm : list) { -// osm.setFiltered(false); -// } -// } + // public void setDisabled(OsmPrimitive... osm) { + // if (osm.length == 1 && osm[0] == null) { + // setDisabled(); + // return; + // } + // clearDisabled(allPrimitives()); + // for (OsmPrimitive o : osm) + // if (o != null) { + // o.setDisabled(true); + // } + // } + // + // public void setDisabled(Collection selection) { + // clearDisabled(nodes); + // clearDisabled(ways); + // clearDisabled(relations); + // for (OsmPrimitive osm : selection) { + // osm.setDisabled(true); + // } + // } + // + // /** + // * Remove the disabled parameter from every value in the collection. + // * @param list The collection to remove the disabled parameter from. + // */ + // private void clearDisabled(Collection list) { + // for (OsmPrimitive osm : list) { + // osm.setDisabled(false); + // } + // } + // + // + // public void setFiltered(Collection selection) { + // clearFiltered(nodes); + // clearFiltered(ways); + // clearFiltered(relations); + // for (OsmPrimitive osm : selection) { + // osm.setFiltered(true); + // } + // } + // + // public void setFiltered(OsmPrimitive... osm) { + // if (osm.length == 1 && osm[0] == null) { + // setFiltered(); + // return; + // } + // clearFiltered(nodes); + // clearFiltered(ways); + // clearFiltered(relations); + // for (OsmPrimitive o : osm) + // if (o != null) { + // o.setFiltered(true); + // } + // } + // + // /** + // * Remove the filtered parameter from every value in the collection. + // * @param list The collection to remove the filtered parameter from. + // */ + // private void clearFiltered(Collection list) { + // if (list == null) + // return; + // for (OsmPrimitive osm : list) { + // osm.setFiltered(false); + // } + // } @Override public DataSet clone() { - DataSet ds = new DataSet(); - HashMap primitivesMap = new HashMap(); - for (Node n : nodes) { - Node newNode = new Node(n); - primitivesMap.put(n, newNode); - ds.addPrimitive(newNode); - } - for (Way w : ways) { - Way newWay = new Way(w); - primitivesMap.put(w, newWay); - List newNodes = new ArrayList(); - for (Node n: w.getNodes()) { - newNodes.add((Node)primitivesMap.get(n)); - } - newWay.setNodes(newNodes); - ds.addPrimitive(newWay); - } - // Because relations can have other relations as members we first clone all relations - // and then get the cloned members - for (Relation r : relations) { - Relation newRelation = new Relation(r, r.isNew()); - newRelation.setMembers(null); - primitivesMap.put(r, newRelation); - ds.addPrimitive(newRelation); - } - for (Relation r : relations) { - Relation newRelation = (Relation)primitivesMap.get(r); - List newMembers = new ArrayList(); - for (RelationMember rm: r.getMembers()) { - newMembers.add(new RelationMember(rm.getRole(), primitivesMap.get(rm.getMember()))); - } - newRelation.setMembers(newMembers); - } - for (DataSource source : dataSources) { - ds.dataSources.add(new DataSource(source.bounds, source.origin)); - } - ds.version = version; - return ds; + getReadLock().lock(); + try { + DataSet ds = new DataSet(); + HashMap primitivesMap = new HashMap(); + for (Node n : nodes) { + Node newNode = new Node(n); + primitivesMap.put(n, newNode); + ds.addPrimitive(newNode); + } + for (Way w : ways) { + Way newWay = new Way(w); + primitivesMap.put(w, newWay); + List newNodes = new ArrayList(); + for (Node n: w.getNodes()) { + newNodes.add((Node)primitivesMap.get(n)); + } + newWay.setNodes(newNodes); + ds.addPrimitive(newWay); + } + // Because relations can have other relations as members we first clone all relations + // and then get the cloned members + for (Relation r : relations) { + Relation newRelation = new Relation(r, r.isNew()); + newRelation.setMembers(null); + primitivesMap.put(r, newRelation); + ds.addPrimitive(newRelation); + } + for (Relation r : relations) { + Relation newRelation = (Relation)primitivesMap.get(r); + List newMembers = new ArrayList(); + for (RelationMember rm: r.getMembers()) { + newMembers.add(new RelationMember(rm.getRole(), primitivesMap.get(rm.getMember()))); + } + newRelation.setMembers(newMembers); + } + for (DataSource source : dataSources) { + ds.dataSources.add(new DataSource(source.bounds, source.origin)); + } + ds.version = version; + return ds; + } finally { + getReadLock().unlock(); + } } @@ -663,5 +705,5 @@ } - protected void deleteWay(Way way) { + private void deleteWay(Way way) { way.setNodes(null); way.setDeleted(true); @@ -674,13 +716,18 @@ */ public void unlinkNodeFromWays(Node node) { - for (Way way: ways) { - List nodes = way.getNodes(); - if (nodes.remove(node)) { - if (nodes.size() < 2) { - deleteWay(way); - } else { - way.setNodes(nodes); + beginUpdate(); + try { + for (Way way: ways) { + List nodes = way.getNodes(); + if (nodes.remove(node)) { + if (nodes.size() < 2) { + deleteWay(way); + } else { + way.setNodes(nodes); + } } } + } finally { + endUpdate(); } } @@ -692,20 +739,25 @@ */ public void unlinkPrimitiveFromRelations(OsmPrimitive primitive) { - for (Relation relation : relations) { - List members = relation.getMembers(); - - Iterator it = members.iterator(); - boolean removed = false; - while(it.hasNext()) { - RelationMember member = it.next(); - if (member.getMember().equals(primitive)) { - it.remove(); - removed = true; + beginUpdate(); + try { + for (Relation relation : relations) { + List members = relation.getMembers(); + + Iterator it = members.iterator(); + boolean removed = false; + while(it.hasNext()) { + RelationMember member = it.next(); + if (member.getMember().equals(primitive)) { + it.remove(); + removed = true; + } } - } - - if (removed) { - relation.setMembers(members); - } + + if (removed) { + relation.setMembers(members); + } + } + } finally { + endUpdate(); } } @@ -718,9 +770,14 @@ */ public void unlinkReferencesToPrimitive(OsmPrimitive referencedPrimitive) { - if (referencedPrimitive instanceof Node) { - unlinkNodeFromWays((Node)referencedPrimitive); - unlinkPrimitiveFromRelations(referencedPrimitive); - } else { - unlinkPrimitiveFromRelations(referencedPrimitive); + beginUpdate(); + try { + if (referencedPrimitive instanceof Node) { + unlinkNodeFromWays((Node)referencedPrimitive); + unlinkPrimitiveFromRelations(referencedPrimitive); + } else { + unlinkPrimitiveFromRelations(referencedPrimitive); + } + } finally { + endUpdate(); } } @@ -804,4 +861,5 @@ */ public void beginUpdate() { + lock.writeLock().lock(); updateCount++; } @@ -814,20 +872,44 @@ updateCount--; if (updateCount == 0) { - fireDataChanged(); - } + List eventsCopy = new ArrayList(cachedEvents); + cachedEvents.clear(); + lock.writeLock().unlock(); + + if (!eventsCopy.isEmpty()) { + lock.readLock().lock(); + try { + if (eventsCopy.size() < MAX_SINGLE_EVENTS) { + for (AbstractDatasetChangedEvent event: eventsCopy) { + fireEventToListeners(event); + } + } else if (eventsCopy.size() == MAX_EVENTS) { + fireEventToListeners(new DataChangedEvent(this)); + } else { + fireEventToListeners(new DataChangedEvent(this, eventsCopy)); + } + } finally { + lock.readLock().unlock(); + } + } + } else { + lock.writeLock().unlock(); + } + } else throw new AssertionError("endUpdate called without beginUpdate"); } + private void fireEventToListeners(AbstractDatasetChangedEvent event) { + for (DataSetListener listener: listeners) { + event.fire(listener); + } + } + private void fireEvent(AbstractDatasetChangedEvent event) { - if (updateCount == 0) { - for (DataSetListener dsl : listeners) { - event.fire(dsl); - } - } - } - - private void fireDataChanged() { - fireEvent(new DataChangedEvent(this)); + if (updateCount == 0) + throw new AssertionError("dataset events can be fired only when dataset is locked"); + if (cachedEvents.size() < MAX_EVENTS) { + cachedEvents.add(event); + } } @@ -868,8 +950,13 @@ public void clenupDeletedPrimitives() { - if (cleanupDeleted(nodes.iterator()) - | cleanupDeleted(ways.iterator()) - | cleanupDeleted(relations.iterator())) { - fireSelectionChanged(); + beginUpdate(); + try { + if (cleanupDeleted(nodes.iterator()) + | cleanupDeleted(ways.iterator()) + | cleanupDeleted(relations.iterator())) { + fireSelectionChanged(); + } + } finally { + endUpdate(); } } @@ -896,12 +983,17 @@ */ public void clear() { - clearSelection(); - for (OsmPrimitive primitive:allPrimitives) { - primitive.setDataset(null); - } - nodes.clear(); - ways.clear(); - relations.clear(); - allPrimitives.clear(); + beginUpdate(); + try { + clearSelection(); + for (OsmPrimitive primitive:allPrimitives) { + primitive.setDataset(null); + } + nodes.clear(); + ways.clear(); + relations.clear(); + allPrimitives.clear(); + } finally { + endUpdate(); + } } } Index: trunk/src/org/openstreetmap/josm/data/osm/Node.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/Node.java (revision 3347) +++ trunk/src/org/openstreetmap/josm/data/osm/Node.java (revision 3348) @@ -20,5 +20,10 @@ if(coor != null){ if (getDataSet() != null) { - getDataSet().fireNodeMoved(this, coor); + boolean locked = writeLock(); + try { + getDataSet().fireNodeMoved(this, coor); + } finally { + writeUnlock(locked); + } } else { setCoorInternal(coor); @@ -115,6 +120,11 @@ @Override public void cloneFrom(OsmPrimitive osm) { - super.cloneFrom(osm); - setCoor(((Node)osm).coor); + boolean locked = writeLock(); + try { + super.cloneFrom(osm); + setCoor(((Node)osm).coor); + } finally { + writeUnlock(locked); + } } @@ -132,13 +142,23 @@ @Override public void mergeFrom(OsmPrimitive other) { - super.mergeFrom(other); - if (!other.isIncomplete()) { - setCoor(new LatLon(((Node)other).coor)); + boolean locked = writeLock(); + try { + super.mergeFrom(other); + if (!other.isIncomplete()) { + setCoor(new LatLon(((Node)other).coor)); + } + } finally { + writeUnlock(locked); } } @Override public void load(PrimitiveData data) { - super.load(data); - setCoor(((NodeData)data).getCoor()); + boolean locked = writeLock(); + try { + super.load(data); + setCoor(((NodeData)data).getCoor()); + } finally { + writeUnlock(locked); + } } Index: trunk/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java (revision 3347) +++ trunk/src/org/openstreetmap/josm/data/osm/OsmPrimitive.java (revision 3348) @@ -345,7 +345,23 @@ } + protected boolean writeLock() { + if (dataSet != null) { + dataSet.beginUpdate(); + return true; + } else + return false; + } + + protected void writeUnlock(boolean locked) { + if (locked) { + // It shouldn't be possible for dataset to become null because method calling setDataset would need write lock which is owned by this thread + dataSet.endUpdate(); + } + } + + /*------------------- * OTHER PROPERTIES - *-------------------/ + *-------------------*/ /** @@ -392,4 +408,5 @@ */ public long getId() { + long id = this.id; return id >= 0?id:0; } @@ -434,18 +451,23 @@ */ public void setOsmId(long id, int version) { - if (id <= 0) - throw new IllegalArgumentException(tr("ID > 0 expected. Got {0}.", id)); - if (version <= 0) - throw new IllegalArgumentException(tr("Version > 0 expected. Got {0}.", version)); - if (dataSet != null && id != this.id) { - DataSet datasetCopy = dataSet; - // Reindex primitive - datasetCopy.removePrimitive(this); + boolean locked = writeLock(); + try { + if (id <= 0) + throw new IllegalArgumentException(tr("ID > 0 expected. Got {0}.", id)); + if (version <= 0) + throw new IllegalArgumentException(tr("Version > 0 expected. Got {0}.", version)); + if (dataSet != null && id != this.id) { + DataSet datasetCopy = dataSet; + // Reindex primitive + datasetCopy.removePrimitive(this); + this.id = id; + datasetCopy.addPrimitive(this); + } this.id = id; - datasetCopy.addPrimitive(this); - } - this.id = id; - this.version = version; - this.setIncomplete(false); + this.version = version; + this.setIncomplete(false); + } finally { + writeUnlock(locked); + } } @@ -462,4 +484,6 @@ if (dataSet != null) throw new DataIntegrityProblemException("Method cannot be called after primitive was added to the dataset"); + + // Not part of dataset - no lock necessary this.id = generateUniqueId(); this.version = 0; @@ -483,5 +507,10 @@ */ public void setUser(User user) { - this.user = user; + boolean locked = writeLock(); + try { + this.user = user; + } finally { + writeUnlock(locked); + } } @@ -506,14 +535,20 @@ */ public void setChangesetId(int changesetId) throws IllegalStateException, IllegalArgumentException { - if (this.changesetId == changesetId) - return; - if (changesetId < 0) - throw new IllegalArgumentException(MessageFormat.format("Parameter ''{0}'' >= 0 expected, got {1}", "changesetId", changesetId)); - if (isNew() && changesetId > 0) - throw new IllegalStateException(tr("Cannot assign a changesetId > 0 to a new primitive. Value of changesetId is {0}", changesetId)); - int old = this.changesetId; - this.changesetId = changesetId; - if (dataSet != null) { - dataSet.fireChangesetIdChanged(this, old, changesetId); + boolean locked = writeLock(); + try { + if (this.changesetId == changesetId) + return; + if (changesetId < 0) + throw new IllegalArgumentException(MessageFormat.format("Parameter ''{0}'' >= 0 expected, got {1}", "changesetId", changesetId)); + if (isNew() && changesetId > 0) + throw new IllegalStateException(tr("Cannot assign a changesetId > 0 to a new primitive. Value of changesetId is {0}", changesetId)); + + int old = this.changesetId; + this.changesetId = changesetId; + if (dataSet != null) { + dataSet.fireChangesetIdChanged(this, old, changesetId); + } + } finally { + writeUnlock(locked); } } @@ -529,5 +564,10 @@ public void setTimestamp(Date timestamp) { - this.timestamp = (int)(timestamp.getTime() / 1000); + boolean locked = writeLock(); + try { + this.timestamp = (int)(timestamp.getTime() / 1000); + } finally { + writeUnlock(locked); + } } @@ -554,5 +594,5 @@ private volatile short flags = FLAG_VISIBLE; // visible per default - private void updateFlags(int flag, boolean value) { + private void updateFlagsNoLock(int flag, boolean value) { if (value) { flags |= flag; @@ -562,4 +602,13 @@ } + private void updateFlags(int flag, boolean value) { + boolean locked = writeLock(); + try { + updateFlagsNoLock(flag, value); + } finally { + writeUnlock(locked); + } + } + /** * Make the primitive disabled (e.g. if a filter applies). @@ -569,9 +618,10 @@ */ public void setDisabledState(boolean hide) { - flags |= FLAG_DISABLED; - if (hide) { - flags |= FLAG_HIDE_IF_DISABLED; - } else { - flags &= ~FLAG_HIDE_IF_DISABLED; + boolean locked = writeLock(); + try { + updateFlagsNoLock(FLAG_DISABLED, true); + updateFlagsNoLock(FLAG_HIDE_IF_DISABLED, hide); + } finally { + writeUnlock(locked); } } @@ -688,7 +738,12 @@ */ public void setVisible(boolean visible) throws IllegalStateException{ - if (isNew() && visible == false) - throw new IllegalStateException(tr("A primitive with ID = 0 cannot be invisible.")); - updateFlags(FLAG_VISIBLE, visible); + boolean locked = writeLock(); + try { + if (isNew() && visible == false) + throw new IllegalStateException(tr("A primitive with ID = 0 cannot be invisible.")); + updateFlagsNoLock(FLAG_VISIBLE, visible); + } finally { + writeUnlock(locked); + } } @@ -701,12 +756,17 @@ */ public void setDeleted(boolean deleted) { - updateFlags(FLAG_DELETED, deleted); - setModified(deleted ^ !isVisible()); - if (dataSet != null) { - if (deleted) { - dataSet.firePrimitivesRemoved(Collections.singleton(this), false); - } else { - dataSet.firePrimitivesAdded(Collections.singleton(this), false); - } + boolean locked = writeLock(); + try { + updateFlagsNoLock(FLAG_DELETED, deleted); + setModified(deleted ^ !isVisible()); + if (dataSet != null) { + if (deleted) { + dataSet.firePrimitivesRemoved(Collections.singleton(this), false); + } else { + dataSet.firePrimitivesAdded(Collections.singleton(this), false); + } + } + } finally { + writeUnlock(locked); } } @@ -718,12 +778,17 @@ */ private void setIncomplete(boolean incomplete) { - if (dataSet != null && incomplete != this.isIncomplete()) { - if (incomplete) { - dataSet.firePrimitivesRemoved(Collections.singletonList(this), true); - } else { - dataSet.firePrimitivesAdded(Collections.singletonList(this), true); - } - } - updateFlags(FLAG_INCOMPLETE, incomplete); + boolean locked = writeLock(); + try { + if (dataSet != null && incomplete != this.isIncomplete()) { + if (incomplete) { + dataSet.firePrimitivesRemoved(Collections.singletonList(this), true); + } else { + dataSet.firePrimitivesAdded(Collections.singletonList(this), true); + } + } + updateFlagsNoLock(FLAG_INCOMPLETE, incomplete); + } finally { + writeUnlock(locked); + } } @@ -842,10 +907,10 @@ for (String key: keySet()) { if (!isUninterestingKey(key)) { - updateFlags(FLAG_TAGGED, true); + updateFlagsNoLock(FLAG_TAGGED, true); return; } } } - updateFlags(FLAG_TAGGED, false); + updateFlagsNoLock(FLAG_TAGGED, false); } @@ -871,6 +936,6 @@ } - updateFlags(FLAG_DIRECTION_REVERSED, directionReversed); - updateFlags(FLAG_HAS_DIRECTIONS, hasDirections); + updateFlagsNoLock(FLAG_DIRECTION_REVERSED, directionReversed); + updateFlagsNoLock(FLAG_HAS_DIRECTIONS, hasDirections); } @@ -890,4 +955,8 @@ ------------*/ + // Note that all methods that read keys first make local copy of keys array reference. This is to ensure thread safety - reading + // doesn't have to be locked so it's possible that keys array will be modified. But all write methods make copy of keys array so + // the array itself will be never modified - only reference will be changed + /** * The key/value list for this primitive. @@ -901,8 +970,8 @@ * @return tags of this primitive. Changes made in returned map are not mapped * back to the primitive, use setKeys() to modify the keys - * @since 1924 */ public Map getKeys() { Map result = new HashMap(); + String[] keys = this.keys; if (keys != null) { for (int i=0; i keys) { - Map originalKeys = getKeys(); - if (keys == null || keys.isEmpty()) { - this.keys = null; + boolean locked = writeLock(); + try { + Map originalKeys = getKeys(); + if (keys == null || keys.isEmpty()) { + this.keys = null; + keysChangedImpl(originalKeys); + return; + } + String[] newKeys = new String[keys.size() * 2]; + int index = 0; + for (Entry entry:keys.entrySet()) { + newKeys[index++] = entry.getKey(); + newKeys[index++] = entry.getValue(); + } + this.keys = newKeys; keysChangedImpl(originalKeys); - return; - } - String[] newKeys = new String[keys.size() * 2]; - int index = 0; - for (Entry entry:keys.entrySet()) { - newKeys[index++] = entry.getKey(); - newKeys[index++] = entry.getValue(); - } - this.keys = newKeys; - keysChangedImpl(originalKeys); + } finally { + writeUnlock(locked); + } } @@ -947,56 +1020,66 @@ */ public final void put(String key, String value) { - Map originalKeys = getKeys(); - if (key == null) - return; - else if (value == null) { - remove(key); - } else if (keys == null){ - keys = new String[] {key, value}; - keysChangedImpl(originalKeys); - } else { - for (int i=0; i originalKeys = getKeys(); + if (key == null) + return; + else if (value == null) { + remove(key); + } else if (keys == null){ + keys = new String[] {key, value}; + keysChangedImpl(originalKeys); + } else { + for (int i=0; i originalKeys = getKeys(); + if (keys.length == 2) { + keys = null; + keysChangedImpl(originalKeys); + return; + } + String[] newKeys = new String[keys.length - 2]; + int j=0; + for (int i=0; i < keys.length; i+=2) { + if (!keys[i].equals(key)) { + newKeys[j++] = keys[i]; + newKeys[j++] = keys[i+1]; + } + } keys = newKeys; keysChangedImpl(originalKeys); - } - } - /** - * Remove the given key from the list - * - * @param key the key to be removed. Ignored, if key is null. - */ - public final void remove(String key) { - if (key == null || keys == null) return; - if (!hasKey(key)) - return; - Map originalKeys = getKeys(); - if (keys.length == 2) { - keys = null; - keysChangedImpl(originalKeys); - return; - } - String[] newKeys = new String[keys.length - 2]; - int j=0; - for (int i=0; i < keys.length; i+=2) { - if (!keys[i].equals(key)) { - newKeys[j++] = keys[i]; - newKeys[j++] = keys[i+1]; - } - } - keys = newKeys; - keysChangedImpl(originalKeys); + } finally { + writeUnlock(locked); + } } @@ -1007,8 +1090,13 @@ */ public final void removeAll() { - if (keys != null) { - Map originalKeys = getKeys(); - keys = null; - keysChangedImpl(originalKeys); + boolean locked = writeLock(); + try { + if (keys != null) { + Map originalKeys = getKeys(); + keys = null; + keysChangedImpl(originalKeys); + } + } finally { + writeUnlock(locked); } } @@ -1022,4 +1110,5 @@ */ public final String get(String key) { + String[] keys = this.keys; if (key == null) return null; @@ -1033,4 +1122,5 @@ public final Collection keySet() { + String[] keys = this.keys; if (keys == null) return Collections.emptySet(); @@ -1068,4 +1158,5 @@ */ public boolean hasKey(String key) { + String[] keys = this.keys; if (key == null) return false; if (keys == null) return false; @@ -1166,4 +1257,5 @@ // when way is cloned checkDataset(); + Object referrers = this.referrers; List result = new ArrayList(); if (referrers != null) { @@ -1202,4 +1294,5 @@ */ public void cloneFrom(OsmPrimitive other) { + // write lock is provided by subclasses if (id != other.id && dataSet != null) throw new DataIntegrityProblemException("Osm id cannot be changed after primitive was added to the dataset"); @@ -1240,17 +1333,22 @@ */ public void mergeFrom(OsmPrimitive other) { - CheckParameterUtil.ensureParameterNotNull(other, "other"); - if (other.isNew() ^ isNew()) - throw new DataIntegrityProblemException(tr("Cannot merge because either of the participating primitives is new and the other is not")); - if (! other.isNew() && other.getId() != id) - throw new DataIntegrityProblemException(tr("Cannot merge primitives with different ids. This id is {0}, the other is {1}", id, other.getId())); - - setKeys(other.getKeys()); - timestamp = other.timestamp; - version = other.version; - setIncomplete(other.isIncomplete()); - flags = other.flags; - user= other.user; - changesetId = other.changesetId; + boolean locked = writeLock(); + try { + CheckParameterUtil.ensureParameterNotNull(other, "other"); + if (other.isNew() ^ isNew()) + throw new DataIntegrityProblemException(tr("Cannot merge because either of the participating primitives is new and the other is not")); + if (! other.isNew() && other.getId() != id) + throw new DataIntegrityProblemException(tr("Cannot merge primitives with different ids. This id is {0}, the other is {1}", id, other.getId())); + + setKeys(other.getKeys()); + timestamp = other.timestamp; + version = other.version; + setIncomplete(other.isIncomplete()); + flags = other.flags; + user= other.user; + changesetId = other.changesetId; + } finally { + writeUnlock(locked); + } } @@ -1313,7 +1411,5 @@ */ public String getName() { - if (get("name") != null) - return get("name"); - return null; + return get("name"); } @@ -1356,4 +1452,5 @@ */ public void load(PrimitiveData data) { + // Write lock is provided by subclasses setKeys(data.getKeys()); setTimestamp(data.getTimestamp()); Index: trunk/src/org/openstreetmap/josm/data/osm/Relation.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/Relation.java (revision 3347) +++ trunk/src/org/openstreetmap/josm/data/osm/Relation.java (revision 3348) @@ -38,16 +38,21 @@ */ public void setMembers(List members) { - for (RelationMember rm:this.members) { - rm.getMember().removeReferrer(this); - } - - if (members != null) { - this.members = members.toArray(new RelationMember[members.size()]); - } - for (RelationMember rm:this.members) { - rm.getMember().addReferrer(this); - } - - fireMembersChanged(); + boolean locked = writeLock(); + try { + for (RelationMember rm:this.members) { + rm.getMember().removeReferrer(this); + } + + if (members != null) { + this.members = members.toArray(new RelationMember[members.size()]); + } + for (RelationMember rm:this.members) { + rm.getMember().addReferrer(this); + } + + fireMembersChanged(); + } finally { + writeUnlock(locked); + } } @@ -64,20 +69,30 @@ public void addMember(RelationMember member) { - RelationMember[] newMembers = new RelationMember[members.length + 1]; - System.arraycopy(members, 0, newMembers, 0, members.length); - newMembers[members.length] = member; - members = newMembers; - member.getMember().addReferrer(this); - fireMembersChanged(); + boolean locked = writeLock(); + try { + RelationMember[] newMembers = new RelationMember[members.length + 1]; + System.arraycopy(members, 0, newMembers, 0, members.length); + newMembers[members.length] = member; + members = newMembers; + member.getMember().addReferrer(this); + fireMembersChanged(); + } finally { + writeUnlock(locked); + } } public void addMember(int index, RelationMember member) { - RelationMember[] newMembers = new RelationMember[members.length + 1]; - System.arraycopy(members, 0, newMembers, 0, index); - System.arraycopy(members, index, newMembers, index + 1, members.length - index); - newMembers[index] = member; - members = newMembers; - member.getMember().addReferrer(this); - fireMembersChanged(); + boolean locked = writeLock(); + try { + RelationMember[] newMembers = new RelationMember[members.length + 1]; + System.arraycopy(members, 0, newMembers, 0, index); + System.arraycopy(members, index, newMembers, index + 1, members.length - index); + newMembers[index] = member; + members = newMembers; + member.getMember().addReferrer(this); + fireMembersChanged(); + } finally { + writeUnlock(locked); + } } @@ -89,12 +104,17 @@ */ public RelationMember setMember(int index, RelationMember member) { - RelationMember originalMember = members[index]; - members[index] = member; - if (originalMember.getMember() != member.getMember()) { - member.getMember().addReferrer(this); - originalMember.getMember().removeReferrer(this); - fireMembersChanged(); - } - return originalMember; + boolean locked = writeLock(); + try { + RelationMember originalMember = members[index]; + members[index] = member; + if (originalMember.getMember() != member.getMember()) { + member.getMember().addReferrer(this); + originalMember.getMember().removeReferrer(this); + fireMembersChanged(); + } + return originalMember; + } finally { + writeUnlock(locked); + } } @@ -105,8 +125,13 @@ */ public RelationMember removeMember(int index) { - List members = getMembers(); - RelationMember result = members.remove(index); - setMembers(members); - return result; + boolean locked = writeLock(); + try { + List members = getMembers(); + RelationMember result = members.remove(index); + setMembers(members); + return result; + } finally { + writeUnlock(locked); + } } @@ -162,22 +187,32 @@ @Override public void cloneFrom(OsmPrimitive osm) { - super.cloneFrom(osm); - // It's not necessary to clone members as RelationMember class is immutable - setMembers(((Relation)osm).getMembers()); + boolean locked = writeLock(); + try { + super.cloneFrom(osm); + // It's not necessary to clone members as RelationMember class is immutable + setMembers(((Relation)osm).getMembers()); + } finally { + writeUnlock(locked); + } } @Override public void load(PrimitiveData data) { - super.load(data); - - RelationData relationData = (RelationData) data; - - List newMembers = new ArrayList(); - for (RelationMemberData member : relationData.getMembers()) { - OsmPrimitive primitive = getDataSet().getPrimitiveById(member); - if (primitive == null) - throw new AssertionError("Data consistency problem - relation with missing member detected"); - newMembers.add(new RelationMember(member.getRole(), primitive)); - } - setMembers(newMembers); + boolean locked = writeLock(); + try { + super.load(data); + + RelationData relationData = (RelationData) data; + + List newMembers = new ArrayList(); + for (RelationMemberData member : relationData.getMembers()) { + OsmPrimitive primitive = getDataSet().getPrimitiveById(member); + if (primitive == null) + throw new AssertionError("Data consistency problem - relation with missing member detected"); + newMembers.add(new RelationMember(member.getRole(), primitive)); + } + setMembers(newMembers); + } finally { + writeUnlock(locked); + } } @@ -228,8 +263,12 @@ public RelationMember firstMember() { if (isIncomplete()) return null; + + RelationMember[] members = this.members; return (members.length == 0) ? null : members[0]; } public RelationMember lastMember() { if (isIncomplete()) return null; + + RelationMember[] members = this.members; return (members.length == 0) ? null : members[members.length - 1]; } @@ -244,25 +283,35 @@ return; - List todelete = new ArrayList(); - for (RelationMember member: members) { - if (member.getMember() == primitive) { - todelete.add(member); - } - } - List members = getMembers(); - members.removeAll(todelete); - setMembers(members); + boolean locked = writeLock(); + try { + List todelete = new ArrayList(); + for (RelationMember member: members) { + if (member.getMember() == primitive) { + todelete.add(member); + } + } + List members = getMembers(); + members.removeAll(todelete); + setMembers(members); + } finally { + writeUnlock(locked); + } } @Override public void setDeleted(boolean deleted) { - for (RelationMember rm:members) { - if (deleted) { - rm.getMember().removeReferrer(this); - } else { - rm.getMember().addReferrer(this); - } - } - super.setDeleted(deleted); + boolean locked = writeLock(); + try { + for (RelationMember rm:members) { + if (deleted) { + rm.getMember().removeReferrer(this); + } else { + rm.getMember().addReferrer(this); + } + } + super.setDeleted(deleted); + } finally { + writeUnlock(locked); + } } @@ -276,13 +325,18 @@ return; - ArrayList todelete = new ArrayList(); - for (RelationMember member: members) { - if (primitives.contains(member.getMember())) { - todelete.add(member); - } - } - List members = getMembers(); - members.removeAll(todelete); - setMembers(members); + boolean locked = writeLock(); + try { + ArrayList todelete = new ArrayList(); + for (RelationMember member: members) { + if (primitives.contains(member.getMember())) { + todelete.add(member); + } + } + List members = getMembers(); + members.removeAll(todelete); + setMembers(members); + } finally { + writeUnlock(locked); + } } @@ -301,4 +355,5 @@ public Set getMemberPrimitives() { HashSet ret = new HashSet(); + RelationMember[] members = this.members; for (RelationMember m: members) { if (m.getMember() != null) { @@ -315,4 +370,6 @@ @Override public BBox getBBox() { + RelationMember[] members = this.members; + if (members.length == 0) return new BBox(0, 0, 0, 0); @@ -334,4 +391,6 @@ return null; visitedRelations.add(this); + + RelationMember[] members = this.members; if (members.length == 0) return null; @@ -367,4 +426,5 @@ DataSet dataSet = getDataSet(); if (dataSet != null) { + RelationMember[] members = this.members; for (RelationMember rm: members) { if (rm.getMember().getDataSet() != dataSet) @@ -393,4 +453,5 @@ */ public boolean hasIncompleteMembers() { + RelationMember[] members = this.members; for (RelationMember rm: members) { if (rm.getMember().isIncomplete()) return true; @@ -407,4 +468,5 @@ public Collection getIncompleteMembers() { Set ret = new HashSet(); + RelationMember[] members = this.members; for (RelationMember rm: members) { if (!rm.getMember().isIncomplete()) { Index: trunk/src/org/openstreetmap/josm/data/osm/Way.java =================================================================== --- trunk/src/org/openstreetmap/josm/data/osm/Way.java (revision 3347) +++ trunk/src/org/openstreetmap/josm/data/osm/Way.java (revision 3348) @@ -47,19 +47,24 @@ */ public void setNodes(List nodes) { - for (Node node:this.nodes) { - node.removeReferrer(this); - } - - if (nodes == null) { - this.nodes = new Node[0]; - } else { - this.nodes = nodes.toArray(new Node[nodes.size()]); - } - for (Node node:this.nodes) { - node.addReferrer(this); - } - - clearCached(); - fireNodesChanged(); + boolean locked = writeLock(); + try { + for (Node node:this.nodes) { + node.removeReferrer(this); + } + + if (nodes == null) { + this.nodes = new Node[0]; + } else { + this.nodes = nodes.toArray(new Node[nodes.size()]); + } + for (Node node:this.nodes) { + node.addReferrer(this); + } + + clearCached(); + fireNodesChanged(); + } finally { + writeUnlock(locked); + } } @@ -98,4 +103,6 @@ public boolean containsNode(Node node) { if (node == null) return false; + + Node[] nodes = this.nodes; for (int i=0; i> getNodePairs(boolean sort) { - ArrayList> chunkSet = new ArrayList>(); + public List> getNodePairs(boolean sort) { + List> chunkSet = new ArrayList>(); if (isIncomplete()) return chunkSet; Node lastN = null; - for (Node n : this.nodes) { + Node[] nodes = this.nodes; + for (Node n : nodes) { if (lastN == null) { lastN = n; @@ -198,17 +206,22 @@ @Override public void load(PrimitiveData data) { - super.load(data); - - WayData wayData = (WayData) data; - - List newNodes = new ArrayList(wayData.getNodes().size()); - for (Long nodeId : wayData.getNodes()) { - Node node = (Node)getDataSet().getPrimitiveById(nodeId, OsmPrimitiveType.NODE); - if (node != null) { - newNodes.add(node); - } else - throw new AssertionError("Data consistency problem - way with missing node detected"); - } - setNodes(newNodes); + boolean locked = writeLock(); + try { + super.load(data); + + WayData wayData = (WayData) data; + + List newNodes = new ArrayList(wayData.getNodes().size()); + for (Long nodeId : wayData.getNodes()) { + Node node = (Node)getDataSet().getPrimitiveById(nodeId, OsmPrimitiveType.NODE); + if (node != null) { + newNodes.add(node); + } else + throw new AssertionError("Data consistency problem - way with missing node detected"); + } + setNodes(newNodes); + } finally { + writeUnlock(locked); + } } @@ -223,7 +236,12 @@ @Override public void cloneFrom(OsmPrimitive osm) { - super.cloneFrom(osm); - Way otherWay = (Way)osm; - setNodes(otherWay.getNodes()); + boolean locked = writeLock(); + try { + super.cloneFrom(osm); + Way otherWay = (Way)osm; + setNodes(otherWay.getNodes()); + } finally { + writeUnlock(locked); + } } @@ -256,25 +274,35 @@ public void removeNode(Node n) { if (isIncomplete()) return; - boolean closed = (lastNode() == n && firstNode() == n); - int i; - List copy = getNodes(); - while ((i = copy.indexOf(n)) >= 0) { - copy.remove(i); - } - i = copy.size(); - if (closed && i > 2) { - copy.add(copy.get(0)); - } else if (i >= 2 && i <= 3 && copy.get(0) == copy.get(i-1)) { - copy.remove(i-1); - } - setNodes(copy); + boolean locked = writeLock(); + try { + boolean closed = (lastNode() == n && firstNode() == n); + int i; + List copy = getNodes(); + while ((i = copy.indexOf(n)) >= 0) { + copy.remove(i); + } + i = copy.size(); + if (closed && i > 2) { + copy.add(copy.get(0)); + } else if (i >= 2 && i <= 3 && copy.get(0) == copy.get(i-1)) { + copy.remove(i-1); + } + setNodes(copy); + } finally { + writeUnlock(locked); + } } public void removeNodes(Collection selection) { if (isIncomplete()) return; - for(OsmPrimitive p : selection) { - if (p instanceof Node) { - removeNode((Node)p); - } + boolean locked = writeLock(); + try { + for(OsmPrimitive p : selection) { + if (p instanceof Node) { + removeNode((Node)p); + } + } + } finally { + writeUnlock(locked); } } @@ -289,13 +317,19 @@ public void addNode(Node n) throws IllegalStateException { if (n==null) return; - if (isIncomplete()) - throw new IllegalStateException(tr("Cannot add node {0} to incomplete way {1}.", n.getId(), getId())); - clearCached(); - n.addReferrer(this); - Node[] newNodes = new Node[nodes.length + 1]; - System.arraycopy(nodes, 0, newNodes, 0, nodes.length); - newNodes[nodes.length] = n; - nodes = newNodes; - fireNodesChanged(); + + boolean locked = writeLock(); + try { + if (isIncomplete()) + throw new IllegalStateException(tr("Cannot add node {0} to incomplete way {1}.", n.getId(), getId())); + clearCached(); + n.addReferrer(this); + Node[] newNodes = new Node[nodes.length + 1]; + System.arraycopy(nodes, 0, newNodes, 0, nodes.length); + newNodes[nodes.length] = n; + nodes = newNodes; + fireNodesChanged(); + } finally { + writeUnlock(locked); + } } @@ -311,35 +345,50 @@ public void addNode(int offs, Node n) throws IllegalStateException, IndexOutOfBoundsException { if (n==null) return; - if (isIncomplete()) - throw new IllegalStateException(tr("Cannot add node {0} to incomplete way {1}.", n.getId(), getId())); - clearCached(); - n.addReferrer(this); - Node[] newNodes = new Node[nodes.length + 1]; - System.arraycopy(nodes, 0, newNodes, 0, offs); - System.arraycopy(nodes, offs, newNodes, offs + 1, nodes.length - offs); - newNodes[offs] = n; - nodes = newNodes; - fireNodesChanged(); + + boolean locked = writeLock(); + try { + if (isIncomplete()) + throw new IllegalStateException(tr("Cannot add node {0} to incomplete way {1}.", n.getId(), getId())); + + clearCached(); + n.addReferrer(this); + Node[] newNodes = new Node[nodes.length + 1]; + System.arraycopy(nodes, 0, newNodes, 0, offs); + System.arraycopy(nodes, offs, newNodes, offs + 1, nodes.length - offs); + newNodes[offs] = n; + nodes = newNodes; + fireNodesChanged(); + } finally { + writeUnlock(locked); + } } @Override public void setDeleted(boolean deleted) { - for (Node n:nodes) { - if (deleted) { - n.removeReferrer(this); - } else { - n.addReferrer(this); - } - } - fireNodesChanged(); - super.setDeleted(deleted); + boolean locked = writeLock(); + try { + for (Node n:nodes) { + if (deleted) { + n.removeReferrer(this); + } else { + n.addReferrer(this); + } + } + fireNodesChanged(); + super.setDeleted(deleted); + } finally { + writeUnlock(locked); + } } public boolean isClosed() { if (isIncomplete()) return false; - return nodes.length >= 3 && lastNode() == firstNode(); + + Node[] nodes = this.nodes; + return nodes.length >= 3 && nodes[nodes.length-1] == nodes[0]; } public Node lastNode() { + Node[] nodes = this.nodes; if (isIncomplete() || nodes.length == 0) return null; return nodes[nodes.length-1]; @@ -347,4 +396,5 @@ public Node firstNode() { + Node[] nodes = this.nodes; if (isIncomplete() || nodes.length == 0) return null; return nodes[0]; @@ -352,6 +402,7 @@ public boolean isFirstLastNode(Node n) { + Node[] nodes = this.nodes; if (isIncomplete() || nodes.length == 0) return false; - return n == firstNode() || n == lastNode(); + return n == nodes[0] || n == nodes[nodes.length -1]; } @@ -368,4 +419,5 @@ DataSet dataSet = getDataSet(); if (dataSet != null) { + Node[] nodes = this.nodes; for (Node n: nodes) { if (n.getDataSet() != dataSet) @@ -412,4 +464,5 @@ public boolean hasIncompleteNodes() { + Node[] nodes = this.nodes; for (Node node:nodes) { if (node.isIncomplete())