Bug 578618 - improve MarkerAttributeMap performance. * avoid temporary intern() of arguments * thread safety by using a copy on write Map. * all bulk puts avoid copies for every entry * hashed map by using IdentityHashMap for performance && small memory. * use the IntegerCache of Integer.valueOf() * use Boolean.valueOf() This implementation does not longer expose the Map interface as it would allow to insert null key or values or non interned keys via the iterator if not a specific entrySet is implemented. The public API only publishes Map copies via toMap(). However entrySet() does now conform to the java.util.Map specification: The returned collection will be bound to this map and will remain in sync with this map - however it is not used in that way. Change-Id: I7dfcc12af36cbf19c99176c7f6d0bae00523be67 Signed-off-by: Joerg Kubitz <jkubitz-eclipse@gmx.de> Reviewed-on: https://git.eclipse.org/r/c/platform/eclipse.platform.resources/+/190487 Tested-by: Platform Bot <platform-bot@eclipse.org>
diff --git a/bundles/org.eclipse.core.resources.spysupport/src/org/eclipse/core/internal/resources/SpySupport.java b/bundles/org.eclipse.core.resources.spysupport/src/org/eclipse/core/internal/resources/SpySupport.java index 414475a..e4666a8 100644 --- a/bundles/org.eclipse.core.resources.spysupport/src/org/eclipse/core/internal/resources/SpySupport.java +++ b/bundles/org.eclipse.core.resources.spysupport/src/org/eclipse/core/internal/resources/SpySupport.java
@@ -13,7 +13,8 @@ *******************************************************************************/ package org.eclipse.core.internal.resources; -import java.util.Map; +import java.util.*; +import java.util.Map.Entry; import org.eclipse.core.internal.utils.Cache; import org.eclipse.core.internal.watson.ElementTree; import org.eclipse.core.resources.IResource; @@ -62,8 +63,13 @@ public static IMarkerSetElement[] getElements(MarkerSet markerSet) { return markerSet.elements; } - public static Object[] getElements(MarkerAttributeMap<?> markerMap) { - return markerMap.elements; + public static Object[] getElements(MarkerAttributeMap markerMap) { + ArrayList<Object> legacyElements = new ArrayList<>(); + for (Entry<String, ?> e : markerMap.entrySet()) { + legacyElements.add(e.getKey()); + legacyElements.add(e.getValue()); + } + return legacyElements.toArray(); } public static boolean isContentDescriptionCached(File file) { ResourceInfo info = file.getResourceInfo(false, false);
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Marker.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Marker.java index aaa5a24..6bc2ede 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Marker.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Marker.java
@@ -39,10 +39,10 @@ public class Marker extends PlatformObject implements IMarker { /** Marker identifier. */ - protected long id; + protected final long id; /** Resource with which this marker is associated. */ - protected IResource resource; + protected final IResource resource; /** * Constructs a new marker object. @@ -54,26 +54,6 @@ } /** - * Used internally by {@link Resource#createMarker(String, Map)} to create a - * marker with given attributes - * - * @param resource non null parent resource - * @param markerInfo non null marker info just created for that marker - * @param attributes may be null or empty - */ - Marker(Resource resource, MarkerInfo markerInfo, Map<String, ? extends Object> attributes) { - this(resource, markerInfo.getId()); - if (attributes != null && !attributes.isEmpty()) { - MarkerManager manager = getWorkspace().getMarkerManager(); - boolean validate = manager.isPersistentType(markerInfo.getType()); - markerInfo.setAttributes(attributes, validate); - if (manager.isPersistent(markerInfo)) { - resource.getResourceInfo(false, true).set(ICoreConstants.M_MARKERS_SNAP_DIRTY); - } - } - } - - /** * Checks the given marker info to ensure that it is not null. * Throws an exception if it is. */ @@ -304,6 +284,7 @@ } /** + * adds all Entries * @see IMarker#setAttributes(String[], Object[]) */ @Override @@ -322,7 +303,7 @@ boolean needDelta = !manager.hasDelta(resource.getFullPath(), id); MarkerInfo oldInfo = needDelta ? (MarkerInfo) markerInfo.clone() : null; boolean validate = manager.isPersistentType(markerInfo.getType()); - markerInfo.setAttributes(attributeNames, values, validate); + markerInfo.addAttributes(attributeNames, values, validate); if (manager.isPersistent(markerInfo)) ((Resource) resource).getResourceInfo(false, true).set(ICoreConstants.M_MARKERS_SNAP_DIRTY); if (needDelta) { @@ -335,6 +316,7 @@ } /** + * clears current map and puts entries * @see IMarker#setAttributes(Map) */ @Override
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerAttributeMap.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerAttributeMap.java index 1d4ed79..46cf593 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerAttributeMap.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerAttributeMap.java
@@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2015 IBM Corporation and others. + * Copyright (c) 2000, 2022 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -12,30 +12,31 @@ * IBM Corporation - initial API and implementation * James Blackburn (Broadcom Corp.) - ongoing development * Lars Vogel <Lars.Vogel@vogella.com> - Bug 473427 + * Joerg Kubitz - redesign *******************************************************************************/ package org.eclipse.core.internal.resources; import java.util.*; +import java.util.Map.Entry; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.core.internal.utils.IStringPoolParticipant; import org.eclipse.core.internal.utils.StringPool; /** - * A specialized map implementation that is optimized for a - * small set of interned strings as keys. The provided keys - * MUST be instances of java.lang.String. + * A specialized Map<String,Object> implementation that is optimized for a small + * set of strings as keys. The keys will be interned() on insert. * - * Implemented as a single array that alternates keys and values. + * Unlike a java.util.HashMap nulls are neither allowed for key or value. */ -@SuppressWarnings("unchecked") -public class MarkerAttributeMap<V> implements Map<String, V>, IStringPoolParticipant { - protected Object[] elements = null; - protected int count = 0; +// the Map interface is not implemented as it would allow to insert null key or values +// or non interned keys via the iterator if not a specific entrySet is implemented. +public class MarkerAttributeMap implements IStringPoolParticipant { + // This implementation is a copy on write map. + private final AtomicReference<Map<String, Object>> mapRef; - // 8 attribute keys, 8 attribute values - protected static final int DEFAULT_SIZE = 16; - protected static final int GROW_SIZE = 10; - - private static final Object[] EMPTY = new Object[0]; + // Typically contains 9 keys: + // "severity","sourceId","charStart","charEnd","arguments","id","message","lineNumber","categoryId" + protected static final int DEFAULT_SIZE = 9; /** * Creates a new marker attribute map of default size @@ -46,253 +47,134 @@ /** * Creates a new marker attribute map. - * @param initialCapacity The initial number of elements that will fit in the map. + * + * @param initialCapacity The initial number of elements that will fit in the + * map. */ public MarkerAttributeMap(int initialCapacity) { - elements = initialCapacity > 0 ? new Object[initialCapacity * 2] : EMPTY; + // ignore initialCapacity - a copy on write datastructure will be copied anyway. + mapRef = new AtomicReference<>(Map.of()); } /** - * Creates a new marker attribute map of default size - * @param map The entries in the given map will be added to the new map. + * Copy constructor. Note that a java.util.Map can not be passed since it could + * contain null keys or null values, or keys that are not interned. */ - public MarkerAttributeMap(Map<String, ? extends V> map) { - this(map.size()); - putAll(map); - } - - @Override - public void clear() { - count = 0; - elements = EMPTY; - } - - @Override - public boolean containsKey(Object key) { - if (count == 0) - return false; - key = ((String) key).intern(); - for (int i = 0; i < elements.length; i = i + 2) - if (elements[i] == key) - return true; - return false; - } - - @Override - public boolean containsValue(Object value) { - if (count == 0) - return false; - for (int i = 1; i < elements.length; i = i + 2) - if (elements[i] != null && elements[i].equals(value)) - return true; - return false; + public MarkerAttributeMap(MarkerAttributeMap m) { + mapRef = new AtomicReference<>(copy(m.getMap())); } /** - * This implementation does not conform properly to the specification - * in the Map interface. The returned collection will not be bound to - * this map and will not remain in sync with this map. + * Copy constructor. Entries with null keys are not allowed. Entries with null + * values are silently ignored. */ - @Override - public Set<Entry<String, V>> entrySet() { - return toHashMap().entrySet(); - } - - @Override - public boolean equals(Object o) { - if (!(o instanceof Map)) - return false; - Map<String, V> other = (Map<String, V>) o; - //must be same size - if (count != other.size()) - return false; - - if (count == 0) - return true; - - //keysets must be equal - if (!keySet().equals(other.keySet())) - return false; - - //values for each key must be equal - for (int i = 0; i < elements.length; i = i + 2) { - if (elements[i] != null && (!elements[i + 1].equals(other.get(elements[i])))) - return false; - } - return true; - } - - @Override - public V get(Object key) { - if (count == 0) - return null; - key = ((String) key).intern(); - for (int i = 0; i < elements.length; i = i + 2) - if (elements[i] == key) - return (V) elements[i + 1]; - return null; + public MarkerAttributeMap(Map<String, ? extends Object> map, boolean validate) { + mapRef = new AtomicReference<>(copy(map, validate)); } /** - * The capacity of the map has been exceeded, grow the array by - * GROW_SIZE to accomodate more entries. + * delete all previous values and replace with given map. Entries with null keys + * are not allowed. Entries with null values are silently ignored. */ - protected void grow() { - Object[] expanded = new Object[elements.length + GROW_SIZE]; - System.arraycopy(elements, 0, expanded, 0, elements.length); - elements = expanded; + public void setAttributes(Map<String, ? extends Object> map, boolean validate) { + mapRef.set(copy(map, validate)); } - @Override - public int hashCode() { - int hash = 0; - if (count == 0) - return hash; - for (int i = 0; i < elements.length; i = i + 2) { - if (elements[i] != null) { - hash += elements[i].hashCode(); - } - } - return hash; - } - - @Override - public boolean isEmpty() { - return count == 0; + private Map<String, Object> copy(Map<String, ? extends Object> map, boolean validate) { + Map<String, Object> target = new IdentityHashMap<>(); + putAll(target, map, validate); + return target; } /** - * This implementation does not conform properly to the specification - * in the Map interface. The returned collection will not be bound to - * this map and will not remain in sync with this map. + * puts all entries of the given map. Entries with null keys are not allowed. + * Entries with null values are silently ignored. */ - @Override - public Set<String> keySet() { - Set<String> result = new HashSet<>(size()); - if (count == 0) - return result; - for (int i = 0; i < elements.length; i = i + 2) { - if (elements[i] != null) { - result.add((String) elements[i]); - } - } - return result; + public void putAll(Map<String, ? extends Object> map, boolean validate) { + mapRef.getAndUpdate(old -> { + Map<String, Object> copy = copy(old); + putAll(copy, map, validate); + return copy; + }); } - @Override - public V put(String k, V value) { - if (k == null) - throw new NullPointerException(); - if (value == null) - return remove(k); - String key = k.intern(); - - if (elements.length <= (count * 2)) - grow(); - - // handle the case where we don't have any attributes yet - if (count == 0) { - elements[0] = key; - elements[1] = value; - count++; - return null; - } - - // replace existing value if it exists - for (int i = 0; i < elements.length; i = i + 2) { - if (elements[i] == key) { - Object oldValue = elements[i + 1]; - elements[i + 1] = value; - return (V) oldValue; + private void putAll(Map<String, Object> target, Map<String, ? extends Object> source, boolean validate) { + for (Entry<String, ? extends Object> e : source.entrySet()) { + String key = e.getKey(); + Objects.requireNonNull(key, "insert of null key not allowed"); //$NON-NLS-1$ + Object value = e.getValue(); + if (validate) { + value = MarkerInfo.checkValidAttribute(value); + } + if (value != null) { // null values => ignore + target.put(e.getKey().intern(), value); } } - - // otherwise add it to the list of elements. - for (int i = 0; i < elements.length; i = i + 2) { - if (elements[i] == null) { - elements[i] = key; - elements[i + 1] = value; - count++; - return null; - } - } - return null; } - @Override - public void putAll(Map<? extends String, ? extends V> map) { - for (Map.Entry<? extends String, ? extends V> e : map.entrySet()) - put(e.getKey(), e.getValue()); + private Map<String, Object> copy(Map<String, ? extends Object> map) { + return new IdentityHashMap<>(map); } - @Override - public V remove(Object key) { - if (count == 0) - return null; - key = ((String) key).intern(); - for (int i = 0; i < elements.length; i = i + 2) { - if (elements[i] == key) { - elements[i] = null; - Object result = elements[i + 1]; - elements[i + 1] = null; - count--; - return (V) result; - } - } - return null; + private Map<String, Object> getMap() { + return mapRef.get(); } - @Override - public int size() { - return count; + /** creates a copy that fulfills the java.util.Map interface **/ + public Map<String, Object> toMap() { + return copy(this.getMap()); + } + + /** @see java.util.Map#entrySet **/ + public Set<Map.Entry<String, Object>> entrySet() { + return getMap().entrySet(); + } + + /** + * like {@link java.util.Map#put(Object, Object)} but null keys or values are + * not allowed + */ + public void put(String k, Object value) { + Objects.requireNonNull(k, "insert of null key not allowed"); //$NON-NLS-1$ + Objects.requireNonNull(value, "insert of null value not allowed"); //$NON-NLS-1$ + mapRef.getAndUpdate(map -> { + Map<String, Object> m = copy(map); + m.put(k.intern(), value); + return m; + }); } @Override public void shareStrings(StringPool set) { - //copy elements for thread safety - Object[] array = elements; - if (array == null) - return; - //don't share keys because they are already interned - for (int i = 1; i < array.length; i = i + 2) { - Object o = array[i]; - if (o instanceof String) - array[i] = set.add((String) o); - else if (o instanceof IStringPoolParticipant) + // don't share keys because they are already interned + for (java.util.Map.Entry<String, Object> e : getMap().entrySet()) { + Object o = e.getValue(); + if (o instanceof String) { + getMap().put(e.getKey(), set.add((String) o)); + } else if (o instanceof IStringPoolParticipant) { ((IStringPoolParticipant) o).shareStrings(set); + } } } - /** - * Creates a new hash map with the same contents as this map. - */ - private HashMap<String, V> toHashMap() { - HashMap<String, V> result = new HashMap<>(size()); - if (count == 0) - return result; - for (int i = 0; i < elements.length; i = i + 2) { - if (elements[i] != null) { - result.put((String) elements[i], (V) elements[i + 1]); - } - } - return result; + /** @see java.util.Map#isEmpty **/ + public boolean isEmpty() { + return getMap().isEmpty(); } - /** - * This implementation does not conform properly to the specification - * in the Map interface. The returned collection will not be bound to - * this map and will not remain in sync with this map. - */ - @Override - public Collection<V> values() { - Set<V> result = new HashSet<>(size()); - if (count == 0) - return result; - for (int i = 1; i < elements.length; i = i + 2) { - if (elements[i] != null) { - result.add((V) elements[i]); - } - } - return result; + /** @see java.util.Map#remove **/ + public Object remove(Object key) { + return getMap().remove(key); } + + /** @see java.util.Map#get **/ + public Object get(Object key) { + return getMap().get(key); + } + + /** @see java.util.Map#size **/ + public int size() { + return getMap().size(); + } + }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerInfo.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerInfo.java index 1bbc5b8..d250683 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerInfo.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerInfo.java
@@ -1,5 +1,5 @@ /******************************************************************************* - * Copyright (c) 2000, 2017 IBM Corporation and others. + * Copyright (c) 2000, 2022 IBM Corporation and others. * * This program and the accompanying materials * are made available under the terms of the Eclipse Public License 2.0 @@ -17,32 +17,28 @@ package org.eclipse.core.internal.resources; import java.nio.charset.StandardCharsets; +import java.util.HashMap; import java.util.Map; -import java.util.Map.Entry; import org.eclipse.core.internal.utils.*; import org.eclipse.core.runtime.Assert; import org.eclipse.osgi.util.NLS; public class MarkerInfo implements IMarkerSetElement, Cloneable, IStringPoolParticipant { - - // well known Integer values - protected static final Integer INTEGER_ONE = 1; - protected static final Integer INTEGER_TWO = 2; - protected static final Integer INTEGER_ZERO = 0; - - // - protected static final long UNDEFINED_ID = -1; - /** The store of attributes for this marker. */ - protected Map<String, Object> attributes = null; + // this class is used concurrently => all members have to be final or volatile + /** + * The store of attributes for this marker. Can not be modified since that could + * remove concurrently added entries while the last entry is removed. + */ + private final MarkerAttributeMap attributes; /** The creation time for this marker. */ - protected long creationTime = 0; + protected final long creationTime; /** Marker identifier. */ - protected long id = UNDEFINED_ID; + protected final long id; /** The type of this marker. */ - protected String type = null; + protected volatile String type; /** * Returns whether the given object is a valid attribute value. Returns @@ -66,26 +62,42 @@ } if (value instanceof Boolean) { //return canonical boolean - return ((Boolean) value).booleanValue() ? Boolean.TRUE : Boolean.FALSE; + return Boolean.valueOf(((Boolean) value)); } if (value instanceof Integer) { //replace common integers with canonical values - switch (((Integer) value).intValue()) { - case 0 : - return INTEGER_ZERO; - case 1 : - return INTEGER_ONE; - case 2 : - return INTEGER_TWO; - } - return value; + return Integer.valueOf(((Integer) value)); } //if we got here, it's an invalid attribute value type throw new IllegalArgumentException(NLS.bind(Messages.resources_wrongMarkerAttributeValueType, value.getClass().getName())); } - public MarkerInfo() { + public MarkerInfo(String type, long id) { + this(null, false, type, id); + } + + public MarkerInfo(MarkerAttributeMap map, long creationTime, String type, long id) { super(); + attributes = map; + this.id = id; + this.creationTime = creationTime; + this.type = type; + } + + /** clone constructor **/ + public MarkerInfo(MarkerInfo markerInfo) { + this(markerInfo.attributes, markerInfo.creationTime, markerInfo.type, markerInfo.id); + } + + public MarkerInfo(Map<String, ? extends Object> attributes, boolean validate, long creationTime, String type, + long id) { + this(attributes == null ? new MarkerAttributeMap() : new MarkerAttributeMap(attributes, validate), creationTime, + type, id); + + } + + public MarkerInfo(Map<String, ? extends Object> attributes, boolean validate, String type, long id) { + this(attributes, validate, System.currentTimeMillis(), type, id); } /** @@ -93,29 +105,23 @@ */ @Override public Object clone() { - try { - MarkerInfo copy = (MarkerInfo) super.clone(); - //copy the attribute table contents - copy.attributes = getAttributes(true); - return copy; - } catch (CloneNotSupportedException e) { - //cannot happen because this class implements Cloneable - return null; - } + return new MarkerInfo(this); } public Object getAttribute(String attributeName) { - return attributes == null ? null : attributes.get(attributeName); + return attributes.get(attributeName); } public Map<String, Object> getAttributes() { - return getAttributes(true); + if (attributes.isEmpty()) + return null; + return attributes.toMap(); } - public Map<String, Object> getAttributes(boolean makeCopy) { - if (attributes == null) + public MarkerAttributeMap getAttributes(boolean makeCopy) { + if (attributes.isEmpty()) return null; - return makeCopy ? new MarkerAttributeMap<>(attributes) : attributes; + return makeCopy ? new MarkerAttributeMap(attributes) : attributes; } public Object[] getAttributes(String[] attributeNames) { @@ -138,61 +144,30 @@ return type; } - public void internalSetAttributes(Map<String, Object> map) { - //the cast effectively acts as an assertion to make sure - //the right kind of map is being used - attributes = map; - } - public void setAttribute(String attributeName, Object value, boolean validate) { - if (validate) + if (validate) { value = checkValidAttribute(value); - if (attributes == null) { - if (value == null) - return; - attributes = new MarkerAttributeMap<>(); - attributes.put(attributeName, value); + } + if (value == null) { + attributes.remove(attributeName); } else { - if (value == null) { - attributes.remove(attributeName); - if (attributes.isEmpty()) - attributes = null; - } else { - attributes.put(attributeName, value); - } + attributes.put(attributeName, value); } } + /** deletes previous Attributes **/ public void setAttributes(Map<String, ? extends Object> map, boolean validate) { - if (map == null) - attributes = null; - else { - attributes = new MarkerAttributeMap<>(map.size()); - for (Entry<String, ?> entry : map.entrySet()) { - Object key = entry.getKey(); - Assert.isTrue(key instanceof String); - Object value = entry.getValue(); - setAttribute((String) key, value, validate); - } - } + attributes.setAttributes(map, validate); } - public void setAttributes(String[] attributeNames, Object[] values, boolean validate) { + /** keeps previous Attributes **/ + public void addAttributes(String[] attributeNames, Object[] values, boolean validate) { Assert.isTrue(attributeNames.length == values.length); - for (int i = 0; i < attributeNames.length; i++) - setAttribute(attributeNames[i], values[i], validate); - } - - public void setCreationTime(long value) { - creationTime = value; - } - - public void setId(long value) { - id = value; - } - - public void setType(String value) { - type = value; + Map<String, Object> map = new HashMap<>(); + for (int i = 0; i < attributeNames.length; i++) { + map.put(attributeNames[i], values[i]); + } + attributes.putAll(map, validate); } /* (non-Javadoc @@ -201,8 +176,6 @@ @Override public void shareStrings(StringPool set) { type = set.add(type); - Map<String, Object> map = attributes; - if (map instanceof IStringPoolParticipant) - ((IStringPoolParticipant) map).shareStrings(set); + attributes.shareStrings(set); } }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerManager.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerManager.java index 98be59e..1e99522 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerManager.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerManager.java
@@ -80,14 +80,7 @@ * associated with the specified resource.IMarkerDeltas for Added markers are * generated. */ - private void basicAdd(IResource resource, MarkerSet markers, MarkerInfo newMarker) throws CoreException { - // should always be a new marker. - if (newMarker.getId() != MarkerInfo.UNDEFINED_ID) { - String message = Messages.resources_changeInAdd; - throw new ResourceException( - new ResourceStatus(IResourceStatus.INTERNAL_ERROR, resource.getFullPath(), message)); - } - newMarker.setId(workspace.nextMarkerId()); + private void basicAdd(IResource resource, MarkerSet markers, MarkerInfo newMarker) { markers.add(newMarker); IMarkerSetElement[] changes = new IMarkerSetElement[1]; changes[0] = new MarkerDelta(IResourceDelta.ADDED, resource, newMarker);
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_1.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_1.java index 55c5ae9..559b8a3 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_1.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_1.java
@@ -102,17 +102,17 @@ int attributesSize = input.readInt(); if (attributesSize == 0) return null; - Map<String, Object> result = new MarkerAttributeMap<>(attributesSize); + Map<String, Object> result = new HashMap<>(attributesSize); for (int j = 0; j < attributesSize; j++) { String key = input.readUTF(); int type = input.readInt(); Object value = null; switch (type) { case ATTRIBUTE_INTEGER : - value = input.readInt(); + value = Integer.valueOf(input.readInt()); break; case ATTRIBUTE_BOOLEAN : - value = input.readBoolean(); + value = Boolean.valueOf(input.readBoolean()); break; case ATTRIBUTE_STRING : value = input.readUTF(); @@ -128,24 +128,24 @@ } private MarkerInfo readMarkerInfo(DataInputStream input, List<String> readTypes) throws IOException, CoreException { - MarkerInfo info = new MarkerInfo(); - info.setId(input.readLong()); + long id = input.readLong(); int constant = input.readInt(); + String type = null; switch (constant) { case QNAME : - String type = input.readUTF(); - info.setType(type); + type = input.readUTF(); readTypes.add(type); break; case INDEX : - info.setType(readTypes.get(input.readInt())); + type = readTypes.get(input.readInt()); break; default : //if we get here the marker file is corrupt String msg = Messages.resources_readMarkers; throw new ResourceException(IResourceStatus.FAILED_READ_METADATA, null, msg, null); } - info.internalSetAttributes(readAttributes(input)); - return info; + Map<String, Object> map = readAttributes(input); + long creationTime = 0; + return new MarkerInfo(map, false, creationTime, type, id); } }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_2.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_2.java index 3d7e1d5..ce2009c 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_2.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_2.java
@@ -103,17 +103,17 @@ int attributesSize = input.readShort(); if (attributesSize == 0) return null; - Map<String, Object> result = new MarkerAttributeMap<>(attributesSize); + Map<String, Object> result = new HashMap<>(attributesSize); for (int j = 0; j < attributesSize; j++) { String key = input.readUTF(); byte type = input.readByte(); Object value = null; switch (type) { case ATTRIBUTE_INTEGER : - value = input.readInt(); + value = Integer.valueOf(input.readInt()); break; case ATTRIBUTE_BOOLEAN : - value = input.readBoolean(); + value = Boolean.valueOf(input.readBoolean()); break; case ATTRIBUTE_STRING : value = input.readUTF(); @@ -122,31 +122,32 @@ // do nothing break; } - if (value != null) + if (value != null) { result.put(key, value); + } } return result.isEmpty() ? null : result; } private MarkerInfo readMarkerInfo(DataInputStream input, List<String> readTypes) throws IOException, CoreException { - MarkerInfo info = new MarkerInfo(); - info.setId(input.readLong()); + long id = input.readLong(); byte constant = input.readByte(); + String type = null; switch (constant) { case QNAME : - String type = input.readUTF(); - info.setType(type); + type = input.readUTF(); readTypes.add(type); break; case INDEX : - info.setType(readTypes.get(input.readInt())); + type = readTypes.get(input.readInt()); break; default : //if we get here the marker file is corrupt String msg = Messages.resources_readMarkers; throw new ResourceException(IResourceStatus.FAILED_READ_METADATA, null, msg, null); } - info.internalSetAttributes(readAttributes(input)); - return info; + Map<String, Object> map = readAttributes(input); + long creationTime = 0; + return new MarkerInfo(map, false, creationTime, type, id); } }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_3.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_3.java index 1fe6c0f..e2372f3 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_3.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerReader_3.java
@@ -104,31 +104,18 @@ int attributesSize = input.readShort(); if (attributesSize == 0) return null; - Map<String, Object> result = new MarkerAttributeMap<>(attributesSize); + Map<String, Object> result = new HashMap<>(attributesSize); for (int j = 0; j < attributesSize; j++) { String key = input.readUTF(); byte type = input.readByte(); Object value = null; switch (type) { case ATTRIBUTE_INTEGER : - int intValue = input.readInt(); //canonicalize well known values (marker severity, task priority) - switch (intValue) { - case 0 : - value = MarkerInfo.INTEGER_ZERO; - break; - case 1 : - value = MarkerInfo.INTEGER_ONE; - break; - case 2 : - value = MarkerInfo.INTEGER_TWO; - break; - default : - value = intValue; - } + value = Integer.valueOf(input.readInt()); break; case ATTRIBUTE_BOOLEAN : - value = input.readBoolean(); + value = Boolean.valueOf(input.readBoolean()); break; case ATTRIBUTE_STRING : value = input.readUTF(); @@ -137,32 +124,32 @@ // do nothing break; } - if (value != null) + if (value != null) { result.put(key, value); + } } return result.isEmpty() ? null : result; } private MarkerInfo readMarkerInfo(DataInputStream input, List<String> readTypes) throws IOException, CoreException { - MarkerInfo info = new MarkerInfo(); - info.setId(input.readLong()); + long id = input.readLong(); + String type = null; byte constant = input.readByte(); switch (constant) { case QNAME : - String type = input.readUTF(); - info.setType(type); + type = input.readUTF(); readTypes.add(type); break; case INDEX : - info.setType(readTypes.get(input.readInt())); + type = readTypes.get(input.readInt()); break; default : //if we get here the marker file is corrupt String msg = Messages.resources_readMarkers; throw new ResourceException(IResourceStatus.FAILED_READ_METADATA, null, msg, null); } - info.internalSetAttributes(readAttributes(input)); - info.setCreationTime(input.readLong()); - return info; + Map<String, Object> map = readAttributes(input); + long creationTime = input.readLong(); + return new MarkerInfo(map, false, creationTime, type, id); } }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_1.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_1.java index 7bd9c85..416dff0 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_1.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_1.java
@@ -86,17 +86,17 @@ short attributesSize = input.readShort(); if (attributesSize == 0) return null; - Map<String, Object> result = new MarkerAttributeMap<>(attributesSize); + Map<String, Object> result = new HashMap<>(attributesSize); for (int j = 0; j < attributesSize; j++) { String key = input.readUTF(); byte type = input.readByte(); Object value = null; switch (type) { case ATTRIBUTE_INTEGER : - value = input.readInt(); + value = Integer.valueOf(input.readInt()); break; case ATTRIBUTE_BOOLEAN : - value = input.readBoolean(); + value = Boolean.valueOf(input.readBoolean()); break; case ATTRIBUTE_STRING : value = input.readUTF(); @@ -105,31 +105,33 @@ // do nothing break; } - if (value != null) + if (value != null) { result.put(key, value); + } } return result.isEmpty() ? null : result; } private MarkerInfo readMarkerInfo(DataInputStream input, List<String> readTypes) throws IOException, CoreException { - MarkerInfo info = new MarkerInfo(); - info.setId(input.readLong()); + long id = input.readLong(); byte constant = input.readByte(); + String type = null; switch (constant) { case QNAME : - String type = input.readUTF(); - info.setType(type); + type = input.readUTF(); readTypes.add(type); break; case INDEX : - info.setType(readTypes.get(input.readInt())); + type = readTypes.get(input.readInt()); break; default : //if we get here the marker file is corrupt String msg = Messages.resources_readMarkers; throw new ResourceException(IResourceStatus.FAILED_READ_METADATA, null, msg, null); } - info.internalSetAttributes(readAttributes(input)); + Map<String, Object> map = readAttributes(input); + long creationTime = 0; + MarkerInfo info = new MarkerInfo(map, false, creationTime, type, id); return info; } }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_2.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_2.java index 6707f7f..2e63277 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_2.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerSnapshotReader_2.java
@@ -87,30 +87,17 @@ short attributesSize = input.readShort(); if (attributesSize == 0) return null; - Map<String, Object> result = new MarkerAttributeMap<>(attributesSize); + Map<String, Object> result = new HashMap<>(attributesSize); for (int j = 0; j < attributesSize; j++) { String key = input.readUTF(); byte type = input.readByte(); Object value = null; switch (type) { case ATTRIBUTE_INTEGER : - int intValue = input.readInt(); - switch (intValue) { - case 0 : - value = MarkerInfo.INTEGER_ZERO; - break; - case 1 : - value = MarkerInfo.INTEGER_ONE; - break; - case 2 : - value = MarkerInfo.INTEGER_TWO; - break; - default : - value = intValue; - } + value = Integer.valueOf(input.readInt()); break; case ATTRIBUTE_BOOLEAN : - value = input.readBoolean(); + value = Boolean.valueOf(input.readBoolean()); break; case ATTRIBUTE_STRING : value = input.readUTF(); @@ -119,32 +106,32 @@ // do nothing break; } - if (value != null) + if (value != null) { result.put(key, value); + } } return result.isEmpty() ? null : result; } private MarkerInfo readMarkerInfo(DataInputStream input, List<String> readTypes) throws IOException, CoreException { - MarkerInfo info = new MarkerInfo(); - info.setId(input.readLong()); + long id = input.readLong(); byte constant = input.readByte(); + String type = null; switch (constant) { case QNAME : - String type = input.readUTF(); - info.setType(type); + type = input.readUTF(); readTypes.add(type); break; case INDEX : - info.setType(readTypes.get(input.readInt())); + type = readTypes.get(input.readInt()); break; default : //if we get here the marker file is corrupt String msg = Messages.resources_readMarkers; throw new ResourceException(IResourceStatus.FAILED_READ_METADATA, null, msg, null); } - info.internalSetAttributes(readAttributes(input)); - info.setCreationTime(input.readLong()); - return info; + Map<String, Object> map = readAttributes(input); + long creationTime = input.readLong(); + return new MarkerInfo(map, false, creationTime, type, id); } }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerWriter.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerWriter.java index 9ae305a..4a10fe7 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerWriter.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/MarkerWriter.java
@@ -169,9 +169,9 @@ /* * Write out the given marker attributes to the given output stream. */ - private void write(Map<String, Object> attributes, DataOutputStream output) throws IOException { - output.writeShort(attributes.size()); - for (Map.Entry<String, Object> e : attributes.entrySet()) { + private void write(MarkerAttributeMap markerAttributeMap, DataOutputStream output) throws IOException { + output.writeShort(markerAttributeMap.size()); + for (Map.Entry<String, Object> e : markerAttributeMap.entrySet()) { String key = e.getKey(); output.writeUTF(key); Object value = e.getValue(); @@ -213,10 +213,12 @@ // write out the size of the attribute table and // then each attribute. - if (info.getAttributes(false) == null) { + MarkerAttributeMap attributes = info.getAttributes(false); + if (attributes == null) { output.writeShort(0); - } else - write(info.getAttributes(false), output); + } else { + write(attributes, output); + } // write out the creation time output.writeLong(info.getCreationTime());
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Resource.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Resource.java index 1c70c20..1f7201a 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Resource.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Resource.java
@@ -700,12 +700,17 @@ workspace.prepareOperation(rule, null); checkAccessible(getFlags(getResourceInfo(false, false))); workspace.beginOperation(true); - MarkerInfo info = new MarkerInfo(); - info.setType(type); - info.setCreationTime(System.currentTimeMillis()); - - workspace.getMarkerManager().add(this, info); - return new Marker(this, info, attributes); + long id = workspace.nextMarkerId(); + MarkerManager manager = workspace.getMarkerManager(); + boolean validate = manager.isPersistentType(type); + MarkerInfo markerInfo = new MarkerInfo(attributes, validate, type, id); + manager.add(this, markerInfo); + if (attributes != null && !attributes.isEmpty()) { + if (manager.isPersistent(markerInfo)) { + this.getResourceInfo(false, true).set(ICoreConstants.M_MARKERS_SNAP_DIRTY); + } + } + return new Marker(this, markerInfo.getId()); } finally { workspace.endOperation(rule, false); }
diff --git a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/StringPool.java b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/StringPool.java index 32aafa6..5d0c951 100644 --- a/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/StringPool.java +++ b/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/utils/StringPool.java
@@ -49,13 +49,15 @@ public String add(String string) { if (string == null) return string; - Object result = map.get(string); + String result = map.putIfAbsent(string, string); if (result != null) { - if (result != string) + if (result != string) { + // XXX that number is wrong since String implementation changed to LATIN1 + // encoding, also interned String may have become externed: savings += 44 + 2 * string.length(); - return (String) result; + } + return result; } - map.put(string, string); return string; }
diff --git a/bundles/org.eclipse.core.tools.resources/src/org/eclipse/core/tools/resources/ElementTreeView.java b/bundles/org.eclipse.core.tools.resources/src/org/eclipse/core/tools/resources/ElementTreeView.java index 06df05a..dac2013 100644 --- a/bundles/org.eclipse.core.tools.resources/src/org/eclipse/core/tools/resources/ElementTreeView.java +++ b/bundles/org.eclipse.core.tools.resources/src/org/eclipse/core/tools/resources/ElementTreeView.java
@@ -144,7 +144,7 @@ return count; } - int basicSizeof(MarkerAttributeMap<?> markerMap) { + int basicSizeof(MarkerAttributeMap markerMap) { int count = DeepSize.OBJECT_HEADER_SIZE + 8;//object header plus two slots Object[] elements = SpySupport.getElements(markerMap); if (elements != null) { @@ -277,7 +277,7 @@ if (object instanceof byte[]) return DeepSize.ARRAY_HEADER_SIZE + ((byte[]) object).length; if (object instanceof MarkerAttributeMap) - return basicSizeof((MarkerAttributeMap<?>) object); + return basicSizeof((MarkerAttributeMap) object); if (object instanceof MarkerInfo) return basicSizeof((MarkerInfo) object); if (object instanceof MarkerSet)
diff --git a/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerSetTest.java b/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerSetTest.java index d464661..4c57554 100644 --- a/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerSetTest.java +++ b/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerSetTest.java
@@ -15,6 +15,7 @@ package org.eclipse.core.tests.resources; import java.util.Arrays; +import java.util.Map; import org.eclipse.core.internal.resources.*; import org.eclipse.core.resources.IMarker; @@ -51,9 +52,7 @@ MarkerInfo info = null; MarkerInfo[] infos = new MarkerInfo[max]; for (int i = 0; i < max; i++) { - info = new MarkerInfo(); - info.setId(i); - info.setType(IMarker.PROBLEM); + info = new MarkerInfo(IMarker.PROBLEM, i); info.setAttribute(IMarker.MESSAGE, getRandomString(), true); infos[i] = info; } @@ -83,9 +82,7 @@ MarkerInfo info = null; MarkerInfo[] infos = new MarkerInfo[max]; for (int i = 0; i < max; i++) { - info = new MarkerInfo(); - info.setId(i); - info.setType(IMarker.PROBLEM); + info = new MarkerInfo(IMarker.PROBLEM, i); info.setAttribute(IMarker.MESSAGE, getRandomString(), true); infos[i] = info; } @@ -104,9 +101,7 @@ MarkerInfo info = null; MarkerInfo[] infos = new MarkerInfo[max]; for (int i = 0; i < max; i++) { - info = new MarkerInfo(); - info.setId(i); - info.setType(IMarker.PROBLEM); + info = new MarkerInfo(IMarker.PROBLEM, i); info.setAttribute(IMarker.MESSAGE, getRandomString(), true); infos[i] = info; } @@ -128,4 +123,32 @@ // all gone? assertEquals("3.0", 0, set.size()); } + + public void testMarkerAttributeMap() { + MarkerAttributeMap map = new MarkerAttributeMap(); + String notInternalString = String.valueOf("notIntern".toCharArray()); + assertNotSame(notInternalString.intern(), notInternalString); + map.put(notInternalString, notInternalString); + String key = map.entrySet().iterator().next().getKey(); + assertSame(notInternalString.intern(), key); + try { + map.put(null, 1); + fail("NPE for nul key expected"); + } catch (NullPointerException e) { + // expected + } + try { + map.put("0", null); + fail("NPE for null value expected"); + } catch (NullPointerException e) { + // expected + } + map.put("1", 1); + map.put("2", "2"); + Map<String, Object> map2 = map.toMap(); + assertEquals("2", map2.get("2")); + assertEquals(1, map2.get("1")); + map2.put(null, 1); // allowed for clients using IMarker.getAttributes() + map2.put("0", null);// allowed for clients + } }
diff --git a/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerTest.java b/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerTest.java index d4f24a9..e278bbb 100644 --- a/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerTest.java +++ b/tests/org.eclipse.core.tests.resources/src/org/eclipse/core/tests/resources/MarkerTest.java
@@ -2115,6 +2115,29 @@ } catch (RuntimeException e) { // expected } + try { + Map<String, Object> map2 = marker.getAttributes(); + map2.put("1", null); // allowed for clients using IMarker.getAttributes() + map2.put("2", 2); + marker.setAttributes(map2); + assertNull(marker.getAttribute("1")); + assertEquals(2, marker.getAttribute("2")); + map2.put(null, 1); // allowed for clients using IMarker.getAttributes() + } catch (CoreException e) { + fail("4.24." + resource.getFullPath(), e); + } + try { + Map<String, Object> map2 = marker.getAttributes(); + map2.put(null, 1); // allowed for clients using IMarker.getAttributes() + try { + marker.setAttributes(map2); // not allowed for clients to put null key + fail("4.25"); + } catch (Exception e) { + // expected + } + } catch (CoreException e) { + fail("4.24." + resource.getFullPath(), e); + } } } }