Bug 576333 - [performance] optimize EclipseContext.localValues EclipseContext.isSetLocally is a hotspot during open java editor. Avoid synchronizedMap by using a ConcurrentHashMap for EclipseContext.localValues. Since ConcurrentHashMap does not allow null values it is wrapped. Null values are substituted by a neutral value. Speeds up EclipseContext.localValues by factor ~ 4. Change-Id: Ie79da1a9cfc9b4d7b8bce8a4d590cbc5329183ec Signed-off-by: Joerg Kubitz <jkubitz-eclipse@gmx.de> Reviewed-on: https://git.eclipse.org/r/c/platform/eclipse.platform.runtime/+/185955 Tested-by: Platform Bot <platform-bot@eclipse.org>
diff --git a/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/internal/contexts/ConcurrentNeutralValueMap.java b/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/internal/contexts/ConcurrentNeutralValueMap.java new file mode 100644 index 0000000..f588fac --- /dev/null +++ b/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/internal/contexts/ConcurrentNeutralValueMap.java
@@ -0,0 +1,221 @@ +/******************************************************************************* + * Copyright (c) 2021 Joerg Kubitz. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Joerg Kubitz - initial API and implementation + *******************************************************************************/ +package org.eclipse.e4.core.internal.contexts; + +import java.util.concurrent.ConcurrentHashMap; +import java.util.function.BiConsumer; + +/** + * A wrapper around ConcurrentHashMap which allows null values. + * + * For null values a neutral Element is internally used. + * + * Faster then a synchronized Map. + */ +public class ConcurrentNeutralValueMap<K, V> {// implements subset of Map<K, V> + final ConcurrentHashMap<K, V> delegate = new ConcurrentHashMap<>(); + /** a value that is used for null elements **/ + final private V neutralValue; + + /** + * @param neutralValue a Element that does not equal any other Value + * @see #neutralObject() + */ + public ConcurrentNeutralValueMap(V neutralValue) { + this.neutralValue = neutralValue; + } + + private V wrapValue(V value) { + return value == null ? neutralValue : value; + } + + private V unwrapValue(V v) { + return v == neutralValue ? null : v; + } + + public V get(Object key) { + return unwrapValue(delegate.get(key)); + } + + private static final class NullValue { + @Override + public String toString() { + return "null"; //$NON-NLS-1$ + } + + @Override + public boolean equals(Object obj) { + return false; + } + + @Override + public int hashCode() { + return 0x55555555; + } + } + + /** returns a neutral Element that does not equals any other **/ + public static Object neutralObject() { + return new NullValue(); + } + + /* + * Method overloads from java.lang.Object: + */ + + @Override + public int hashCode() { + return delegate.hashCode(); + } + + @Override + public boolean equals(Object obj) { + return delegate.equals(obj); + } + + @Override + public String toString() { + return delegate.toString(); + } + + /* + * Method overloads from java.util.Map: + */ + + /** + * Like {@link java.util.Map#put} but we do not return a value. + * + * @see java.util.Map#put(Object, Object) + * @see ConcurrentHashMap#put(Object, Object) + * @see #putAndGetOld(Object, Object) + **/ + public void put(K key, V value) { + delegate.put(key, wrapValue(value)); + } + + /** + * Like {@link java.util.Map#put} but we do not return a wrapped value. + * + * @see java.util.Map#put(Object, Object) + * @see Value + **/ + public Value<V> putAndGetOld(K key, V value) { + return new Wrapped(delegate.put(key, wrapValue(value))); + } + + /** + * @see java.util.Map#remove(Object) + * @see ConcurrentHashMap#remove(Object) + **/ + public V remove(Object key) { + return unwrapValue(delegate.remove(key)); + } + + /** + * @see java.util.Map#size() + **/ + public int size() { + return delegate.size(); + } + + /** + * @see java.util.Map#isEmpty() + **/ + public boolean isEmpty() { + return delegate.isEmpty(); + } + + /** + * @see java.util.Map#clear() + **/ + public void clear() { + delegate.clear(); + } + + /** + * Use is discouraged as the outcome might change concurrently + * + * @see java.util.Map#containsKey(Object) + * @see ConcurrentHashMap#containsKey(Object) + **/ + public boolean containsKey(Object key) { + return delegate.containsKey(key); + } + + /* + * Some helpful methods from ConcurrentHashMap which do not return V: + */ + + /** @see ConcurrentHashMap#forEach(BiConsumer) **/ + public void forEach(BiConsumer<? super K, ? super V> action) { + delegate.forEach( // + (k, v) -> action.accept(k, unwrapValue(v) // + )); + } + + /* + * Some methods from ConcurrentHashMap which return V. returning null is + * discouraged because it is not clear if that was a value or not: + */ + + /** + * Like {@link ConcurrentHashMap#putIfAbsent(Object, Object)} but we do not + * return anything. + * + * @see ConcurrentHashMap#putIfAbsent(Object, Object) + **/ + public void putIfAbsent(K key, V value) { + delegate.putIfAbsent(key, wrapValue(value)); + } + + public interface Value<V> { + /** + * @return the result of map.contains(key) + */ + boolean isPresent(); + + /** + * @return the result of map.get(key) + */ + V unwraped(); + } + + private final class Wrapped implements Value<V> { + V wrappedValue; + + private Wrapped(V wrappedValue) { + this.wrappedValue = wrappedValue; + } + + @Override + public boolean isPresent() { + return wrappedValue != null; + } + + @Override + public V unwraped() { + return unwrapValue(wrappedValue); + } + } + + /** + * @return a value that is wrapped into a Value Object. + * @see Value#isPresent() + * @see Value#unwraped() + */ + public Value<V> getValue(K key) { + return new Wrapped(delegate.get(key)); + } + +}
diff --git a/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/internal/contexts/EclipseContext.java b/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/internal/contexts/EclipseContext.java index 5a0f644..6b92681 100644 --- a/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/internal/contexts/EclipseContext.java +++ b/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/internal/contexts/EclipseContext.java
@@ -33,6 +33,7 @@ import org.eclipse.e4.core.contexts.IEclipseContext; import org.eclipse.e4.core.contexts.RunAndTrack; import org.eclipse.e4.core.di.IInjector; +import org.eclipse.e4.core.internal.contexts.ConcurrentNeutralValueMap.Value; import org.eclipse.e4.core.internal.contexts.osgi.ContextDebugHelper; import org.osgi.service.event.Event; import org.osgi.service.event.EventAdmin; @@ -91,7 +92,8 @@ private WeakGroupedListenerList weakListeners = new WeakGroupedListenerList(); private Map<String, ValueComputation> localValueComputations = Collections.synchronizedMap(new HashMap<>()); - final protected Map<String, Object> localValues = Collections.synchronizedMap(new HashMap<>()); + final protected ConcurrentNeutralValueMap<String, Object> localValues = // null values allowed + new ConcurrentNeutralValueMap<>(ConcurrentNeutralValueMap.neutralObject()); private Set<String> modifiable; @@ -256,8 +258,9 @@ Object result = null; // 1. try for local value - if (localValues.containsKey(name)) { - result = localValues.get(name); + Value<Object> value = localValues.getValue(name); + if (value.isPresent()) { + result = value.unwraped(); if (result == null) return null; } else @@ -320,9 +323,10 @@ } protected boolean isLocalEquals(String name, Object newValue) { - if (!localValues.containsKey(name)) + Value<Object> value = localValues.getValue(name); + if (!value.isPresent()) return false; - return (localValues.get(name) == newValue); + return (value.unwraped() == newValue); } private boolean isSetLocally(String name) { @@ -371,8 +375,9 @@ setParent((IEclipseContext) value); return; } - boolean containsKey = localValues.containsKey(name); - Object oldValue = localValues.put(name, value); + Value<Object> old = localValues.putAndGetOld(name, value); + boolean containsKey = old.isPresent(); + Object oldValue = old.unwraped(); if (!containsKey || oldValue != value) { Set<Scheduled> scheduled = new LinkedHashSet<>(); invalidate(name, ContextChangeEvent.ADDED, oldValue, value, scheduled); @@ -407,7 +412,7 @@ String tmp = "Variable " + name + " is not modifiable in the context " + this; //$NON-NLS-1$ //$NON-NLS-2$ throw new IllegalArgumentException(tmp); } - Object oldValue = localValues.put(name, value); + Object oldValue = localValues.putAndGetOld(name, value).unwraped(); if (oldValue != value) invalidate(name, ContextChangeEvent.ADDED, oldValue, value, scheduled); return true; @@ -470,9 +475,7 @@ if (modifiable == null) modifiable = new HashSet<>(3); modifiable.add(name); - if (localValues.containsKey(name)) - return; - localValues.put(name, null); + localValues.putIfAbsent(name, null); } private boolean checkModifiable(String name) { @@ -709,23 +712,22 @@ // This method is for debug only, do not use externally public Map<String, Object> localData() { Map<String, Object> result = new HashMap<>(localValues.size()); - for (Map.Entry<String, Object> entry : localValues.entrySet()) { - if (entry.getValue() instanceof IContextFunction) { - continue; + localValues.forEach((k, v) -> { + if (!(v instanceof IContextFunction)) { + result.put(k, v); } - result.put(entry.getKey(), entry.getValue()); - } + }); return result; } // This method is for debug only, do not use externally public Map<String, Object> localContextFunction() { Map<String, Object> result = new HashMap<>(localValues.size()); - for (Map.Entry<String, Object> entry : localValues.entrySet()) { - if (entry.getValue() instanceof IContextFunction) { - result.put(entry.getKey(), entry.getValue()); + localValues.forEach((k, v) -> { + if (v instanceof IContextFunction) { + result.put(k, v); } - } + }); return result; }
diff --git a/tests/org.eclipse.e4.core.tests/META-INF/MANIFEST.MF b/tests/org.eclipse.e4.core.tests/META-INF/MANIFEST.MF index 3aa9bf4..c9f8b95 100644 --- a/tests/org.eclipse.e4.core.tests/META-INF/MANIFEST.MF +++ b/tests/org.eclipse.e4.core.tests/META-INF/MANIFEST.MF
@@ -2,7 +2,7 @@ Bundle-ManifestVersion: 2 Bundle-Name: E4 Core Tests Bundle-SymbolicName: org.eclipse.e4.core.tests -Bundle-Version: 1.2.200.qualifier +Bundle-Version: 1.2.300.qualifier Bundle-Vendor: Eclipse.org Bundle-Activator: org.eclipse.e4.core.internal.tests.CoreTestsActivator Require-Bundle: org.eclipse.osgi;bundle-version="3.6.0",
diff --git a/tests/org.eclipse.e4.core.tests/pom.xml b/tests/org.eclipse.e4.core.tests/pom.xml index 7ec41c2..cc0712d 100644 --- a/tests/org.eclipse.e4.core.tests/pom.xml +++ b/tests/org.eclipse.e4.core.tests/pom.xml
@@ -19,7 +19,7 @@ </parent> <groupId>org.eclipse.e4</groupId> <artifactId>org.eclipse.e4.core.tests</artifactId> - <version>1.2.200-SNAPSHOT</version> + <version>1.2.300-SNAPSHOT</version> <packaging>eclipse-test-plugin</packaging> <properties>
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/NeutralValueTest.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/NeutralValueTest.java new file mode 100644 index 0000000..dccb2c3 --- /dev/null +++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/internal/tests/contexts/NeutralValueTest.java
@@ -0,0 +1,144 @@ +/******************************************************************************* + * Copyright (c) 2021 Joerg Kubitz. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Joerg Kubitz - initial API and implementation + *******************************************************************************/ +package org.eclipse.e4.core.internal.tests.contexts; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; + +import java.util.HashSet; +import java.util.Set; + +import org.eclipse.e4.core.internal.contexts.ConcurrentNeutralValueMap; +import org.eclipse.e4.core.internal.contexts.ConcurrentNeutralValueMap.Value; +import org.junit.Test; + +public class NeutralValueTest { + + @Test + public void testNeutralObject() { + assertFalse(new Object().equals(ConcurrentNeutralValueMap.neutralObject())); + assertFalse(ConcurrentNeutralValueMap.neutralObject().equals(new Object())); + assertNotEquals(ConcurrentNeutralValueMap.neutralObject(), ConcurrentNeutralValueMap.neutralObject()); + assertEquals("" + null, ConcurrentNeutralValueMap.neutralObject().toString()); + } + + @Test + public void testConcurrentNeutralValueMap() { + ConcurrentNeutralValueMap<String, Double> map = new ConcurrentNeutralValueMap<>(Double.NaN); + map.put("nix", null); // modify + map.put("2", 2.0); // modify + map.put("3", 3.0); // modify + map.put("4", 4.0); // modify + map.put("garnix", null); // modify + assertTrue(map.containsKey("garnix")); + map.remove("garnix"); + + assertTrue(map.containsKey("nix")); + assertTrue(map.containsKey("2")); + assertFalse(map.containsKey("1")); + assertFalse(map.containsKey("garnix")); + + assertEquals(4, map.size()); + + assertFalse(map.isEmpty()); + + assertEquals(null, map.get("nix")); + assertEquals(null, map.get("1")); + assertEquals(Double.valueOf(2.0), map.get("2")); + assertEquals(Double.valueOf(3.0), map.get("3")); + assertEquals(Double.valueOf(4.0), map.get("4")); + + Set<String> keys = new HashSet<>(); + Set<Double> values = new HashSet<>(); + map.forEach((k, v) -> keys.add(k)); + map.forEach((k, v) -> values.add(v)); + assertEquals(Set.of("nix", "2", "3", "4"), keys); + assertTrue(values.contains(null)); + assertTrue(values.contains(2.0)); + + assertTrue(map.getValue("nix").isPresent()); + assertFalse(map.getValue("1").isPresent()); + assertTrue(map.getValue("2").isPresent()); + assertTrue(map.getValue("3").isPresent()); + assertTrue(map.getValue("4").isPresent()); + + { + Value<Double> v = map.getValue("nix"); + assertTrue(v.isPresent()); + assertEquals(null, v.unwraped()); + } + { + Value<Double> v = map.getValue("1"); + assertFalse(v.isPresent()); + assertEquals(null, v.unwraped()); + } + { + Value<Double> v = map.getValue("2"); + assertTrue(v.isPresent()); + assertEquals(Double.valueOf(2.0), v.unwraped()); + } + + { + Value<Double> v = map.putAndGetOld("5", 5555.0); // modify + assertFalse(v.isPresent()); + assertEquals(null, v.unwraped()); + assertEquals(Double.valueOf(5555.0), map.get("5")); + } + { + Value<Double> v = map.putAndGetOld("5", 5.0); // modify + assertTrue(v.isPresent()); + assertEquals(Double.valueOf(5555.0), v.unwraped()); + assertEquals(Double.valueOf(5.0), map.get("5")); + } + map.putIfAbsent("5", 5555.0); // modify + assertEquals(Double.valueOf(5.0), map.get("5")); + map.remove("5"); // modify + assertFalse(map.containsKey("5")); + + { + Value<Double> v = map.putAndGetOld("five", null); // modify + assertFalse(v.isPresent()); + assertEquals(null, v.unwraped()); + assertEquals(null, map.get("five")); + } + { + Value<Double> v = map.putAndGetOld("five", 5.0); // modify + assertTrue(v.isPresent()); + assertEquals(null, v.unwraped()); + assertEquals(Double.valueOf(5.0), map.get("five")); + } + { + map.putIfAbsent("five", null); // modify + Value<Double> v = map.getValue("five"); + assertTrue(v.isPresent()); + assertEquals(Double.valueOf(5.0), v.unwraped()); + } + map.remove("five"); // modify + { + map.putIfAbsent("five", null); // modify + Value<Double> v = map.getValue("five"); + assertTrue(v.isPresent()); + assertEquals(null, v.unwraped()); + } + map.remove("five"); // modify + assertFalse(map.containsKey("five")); + + map.clear(); // modify + assertEquals(0, map.size()); + assertTrue(map.isEmpty()); + } + +}
diff --git a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/tests/CoreTestSuite.java b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/tests/CoreTestSuite.java index 1fe12e8..69a8f39 100644 --- a/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/tests/CoreTestSuite.java +++ b/tests/org.eclipse.e4.core.tests/src/org/eclipse/e4/core/tests/CoreTestSuite.java
@@ -22,6 +22,7 @@ import org.eclipse.e4.core.internal.tests.contexts.ContextDynamicTest; import org.eclipse.e4.core.internal.tests.contexts.DependenciesLeakTest; import org.eclipse.e4.core.internal.tests.contexts.EclipseContextTest; +import org.eclipse.e4.core.internal.tests.contexts.NeutralValueTest; import org.eclipse.e4.core.internal.tests.contexts.ReparentingTest; import org.eclipse.e4.core.internal.tests.contexts.RunAndTrackTest; import org.eclipse.e4.core.internal.tests.contexts.inject.ActivationInjectionTest; @@ -102,6 +103,7 @@ DisposeClassLinkTest.class, InjectStaticContextTest.class, ActivationTest.class, + NeutralValueTest.class, // Contexts injection AnnotationsInjectionTest.class,