From b7ece52d83a0d558b28b5962c63e8eea31f0ecd8 Mon Sep 17 00:00:00 2001 From: Greg Brail Date: Thu, 18 Jul 2024 13:42:26 -0700 Subject: [PATCH] Streamline object operations through a better SlotMap interface Implement a "compute" method which allows a bunch of the logic in ScriptableObject work without as many slot map operations, especially when changing slot instance types. Also, get a particular check out of the slot map implementations that has always bothered me and move it to ScriptableObject for better encapsulation of language-specific behavior. --- benchmarks/build.gradle | 10 +- .../org/mozilla/javascript/AccessorSlot.java | 4 + .../mozilla/javascript/EmbeddedSlotMap.java | 165 +++++++------- .../org/mozilla/javascript/HashSlotMap.java | 39 +--- .../org/mozilla/javascript/LambdaSlot.java | 4 + .../org/mozilla/javascript/LazyLoadSlot.java | 4 + .../mozilla/javascript/ScriptableObject.java | 206 ++++++++++-------- .../java/org/mozilla/javascript/Slot.java | 4 +- .../java/org/mozilla/javascript/SlotMap.java | 24 +- .../mozilla/javascript/SlotMapContainer.java | 9 +- .../ThreadSafeSlotMapContainer.java | 14 +- .../org/mozilla/javascript/SlotMapTest.java | 94 ++++++-- 12 files changed, 315 insertions(+), 262 deletions(-) diff --git a/benchmarks/build.gradle b/benchmarks/build.gradle index d87357bbd4..db9cd8ffb3 100644 --- a/benchmarks/build.gradle +++ b/benchmarks/build.gradle @@ -10,12 +10,12 @@ dependencies { jmh { // use this to include only some - //includes = ['V8'] + //includes = ['SlotMap'] benchmarkMode = ['avgt'] fork = 1 - timeUnit = 'us' - iterations = 5 + //timeUnit = 'ns' + iterations = 3 timeOnIteration = '5s' - warmupIterations = 5 + warmupIterations = 3 warmup = '5s' -} \ No newline at end of file +} diff --git a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java b/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java index b2c94d2335..a12841b8f6 100644 --- a/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java +++ b/rhino/src/main/java/org/mozilla/javascript/AccessorSlot.java @@ -8,6 +8,10 @@ public class AccessorSlot extends Slot { private static final long serialVersionUID = 1677840254177335827L; + AccessorSlot(Object name, int index) { + super(name, index, 0); + } + AccessorSlot(Slot oldSlot) { super(oldSlot); } diff --git a/rhino/src/main/java/org/mozilla/javascript/EmbeddedSlotMap.java b/rhino/src/main/java/org/mozilla/javascript/EmbeddedSlotMap.java index b37befd09a..a0b2db2365 100644 --- a/rhino/src/main/java/org/mozilla/javascript/EmbeddedSlotMap.java +++ b/rhino/src/main/java/org/mozilla/javascript/EmbeddedSlotMap.java @@ -111,10 +111,12 @@ public Slot modify(Object key, int index, int attributes) { } // A new slot has to be inserted. - return createSlot(key, indexOrHash, attributes); + Slot newSlot = new Slot(key, index, attributes); + createNewSlot(newSlot); + return newSlot; } - private Slot createSlot(Object key, int indexOrHash, int attributes) { + private void createNewSlot(Slot newSlot) { if (count == 0) { // Always throw away old slots if any on empty insert. slots = new Slot[INITIAL_SLOT_SIZE]; @@ -128,52 +130,64 @@ private Slot createSlot(Object key, int indexOrHash, int attributes) { slots = newSlots; } - Slot newSlot = new Slot(key, indexOrHash, attributes); insertNewSlot(newSlot); - return newSlot; } @Override - public void replace(Slot oldSlot, Slot newSlot) { - final int insertPos = getSlotIndex(slots.length, oldSlot.indexOrHash); - Slot prev = slots[insertPos]; - Slot tmpSlot = prev; - // Find original slot and previous one in hash table - while (tmpSlot != null) { - if (tmpSlot == oldSlot) { - break; - } - prev = tmpSlot; - tmpSlot = tmpSlot.next; - } - - // It's an error to call this when the slot isn't already there - assert (tmpSlot == oldSlot); - - // add new slot to hash table - if (prev == oldSlot) { - slots[insertPos] = newSlot; - } else { - prev.next = newSlot; - } - newSlot.next = oldSlot.next; + public S compute(Object key, int index, SlotComputer c) { + final int indexOrHash = (key != null ? key.hashCode() : index); - // Replace new slot in linked list, keeping same order - if (oldSlot == firstAdded) { - firstAdded = newSlot; - } else { - Slot ps = firstAdded; - while ((ps != null) && (ps.orderedNext != oldSlot)) { - ps = ps.orderedNext; + if (slots != null) { + Slot slot; + final int slotIndex = getSlotIndex(slots.length, indexOrHash); + Slot prev = slots[slotIndex]; + for (slot = prev; slot != null; slot = slot.next) { + if (indexOrHash == slot.indexOrHash && Objects.equals(slot.name, key)) { + break; + } + prev = slot; } - if (ps != null) { - ps.orderedNext = newSlot; + if (slot != null) { + // Modify or remove existing slot + S newSlot = c.compute(key, index, slot); + if (newSlot == null) { + // Need to delete this slot actually + removeSlot(slot, prev, slotIndex, key); + } else if (!Objects.equals(slot, newSlot)) { + // Replace slot in hash table + if (prev == slot) { + slots[slotIndex] = newSlot; + } else { + prev.next = newSlot; + } + newSlot.next = slot.next; + // Replace new slot in linked list, keeping same order + if (slot == firstAdded) { + firstAdded = newSlot; + } else { + Slot ps = firstAdded; + while ((ps != null) && (ps.orderedNext != slot)) { + ps = ps.orderedNext; + } + if (ps != null) { + ps.orderedNext = newSlot; + } + } + newSlot.orderedNext = slot.orderedNext; + if (slot == lastAdded) { + lastAdded = newSlot; + } + } + return newSlot; } } - newSlot.orderedNext = oldSlot.orderedNext; - if (oldSlot == lastAdded) { - lastAdded = newSlot; + + // If we get here, we know we are potentially adding a new slot + S newSlot = c.compute(key, index, null); + if (newSlot != null) { + createNewSlot(newSlot); } + return newSlot; } @Override @@ -194,62 +208,35 @@ private void insertNewSlot(Slot newSlot) { firstAdded = newSlot; } lastAdded = newSlot; - // add new slot to hash table, return it addKnownAbsentSlot(slots, newSlot); } - @Override - public void remove(Object key, int index) { - int indexOrHash = (key != null ? key.hashCode() : index); + private void removeSlot(Slot slot, Slot prev, int ix, Object key) { + count--; + // remove slot from hash table + if (prev == slot) { + slots[ix] = slot.next; + } else { + prev.next = slot.next; + } - if (count != 0) { - final int slotIndex = getSlotIndex(slots.length, indexOrHash); - Slot prev = slots[slotIndex]; - Slot slot = prev; - while (slot != null) { - if (slot.indexOrHash == indexOrHash && Objects.equals(slot.name, key)) { - break; - } - prev = slot; - slot = slot.next; - } - if (slot != null) { - // non-configurable - if ((slot.getAttributes() & ScriptableObject.PERMANENT) != 0) { - Context cx = Context.getContext(); - if (cx.isStrictMode()) { - throw ScriptRuntime.typeErrorById( - "msg.delete.property.with.configurable.false", key); - } - return; - } - count--; - // remove slot from hash table - if (prev == slot) { - slots[slotIndex] = slot.next; - } else { - prev.next = slot.next; - } + // remove from ordered list. Previously this was done lazily in + // getIds() but delete is an infrequent operation so O(n) + // should be ok - // remove from ordered list. Previously this was done lazily in - // getIds() but delete is an infrequent operation so O(n) - // should be ok - - // ordered list always uses the actual slot - if (slot == firstAdded) { - prev = null; - firstAdded = slot.orderedNext; - } else { - prev = firstAdded; - while (prev.orderedNext != slot) { - prev = prev.orderedNext; - } - prev.orderedNext = slot.orderedNext; - } - if (slot == lastAdded) { - lastAdded = prev; - } + // ordered list always uses the actual slot + if (slot == firstAdded) { + prev = null; + firstAdded = slot.orderedNext; + } else { + prev = firstAdded; + while (prev.orderedNext != slot) { + prev = prev.orderedNext; } + prev.orderedNext = slot.orderedNext; + } + if (slot == lastAdded) { + lastAdded = prev; } } diff --git a/rhino/src/main/java/org/mozilla/javascript/HashSlotMap.java b/rhino/src/main/java/org/mozilla/javascript/HashSlotMap.java index 3b7d861b13..5a65a6787b 100644 --- a/rhino/src/main/java/org/mozilla/javascript/HashSlotMap.java +++ b/rhino/src/main/java/org/mozilla/javascript/HashSlotMap.java @@ -38,24 +38,15 @@ public Slot query(Object key, int index) { @Override public Slot modify(Object key, int index, int attributes) { Object name = makeKey(key, index); - Slot slot = map.get(name); - if (slot != null) { - return slot; - } - - return createSlot(key, index, attributes); + return map.computeIfAbsent(name, n -> new Slot(key, index, attributes)); } + @SuppressWarnings("unchecked") @Override - public void replace(Slot oldSlot, Slot newSlot) { - Object name = makeKey(oldSlot); - map.put(name, newSlot); - } - - private Slot createSlot(Object key, int index, int attributes) { - Slot newSlot = new Slot(key, index, attributes); - add(newSlot); - return newSlot; + public S compute(Object key, int index, SlotComputer c) { + Object name = makeKey(key, index); + Slot ret = map.compute(name, (n, existing) -> c.compute(key, index, existing)); + return (S) ret; } @Override @@ -64,24 +55,6 @@ public void add(Slot newSlot) { map.put(name, newSlot); } - @Override - public void remove(Object key, int index) { - Object name = makeKey(key, index); - Slot slot = map.get(name); - if (slot != null) { - // non-configurable - if ((slot.getAttributes() & ScriptableObject.PERMANENT) != 0) { - Context cx = Context.getContext(); - if (cx.isStrictMode()) { - throw ScriptRuntime.typeErrorById( - "msg.delete.property.with.configurable.false", key); - } - return; - } - map.remove(name); - } - } - @Override public Iterator iterator() { return map.values().iterator(); diff --git a/rhino/src/main/java/org/mozilla/javascript/LambdaSlot.java b/rhino/src/main/java/org/mozilla/javascript/LambdaSlot.java index 9e512a856a..dbfe108e36 100644 --- a/rhino/src/main/java/org/mozilla/javascript/LambdaSlot.java +++ b/rhino/src/main/java/org/mozilla/javascript/LambdaSlot.java @@ -14,6 +14,10 @@ public class LambdaSlot extends Slot { private static final long serialVersionUID = -3046681698806493052L; + LambdaSlot(Object name, int index) { + super(name, index, 0); + } + LambdaSlot(Slot oldSlot) { super(oldSlot); } diff --git a/rhino/src/main/java/org/mozilla/javascript/LazyLoadSlot.java b/rhino/src/main/java/org/mozilla/javascript/LazyLoadSlot.java index 43ba0710d7..e55f5aac8d 100644 --- a/rhino/src/main/java/org/mozilla/javascript/LazyLoadSlot.java +++ b/rhino/src/main/java/org/mozilla/javascript/LazyLoadSlot.java @@ -5,6 +5,10 @@ * functions. It's used to load built-in objects more efficiently. */ public class LazyLoadSlot extends Slot { + LazyLoadSlot(Object name, int index) { + super(name, index, 0); + } + LazyLoadSlot(Slot oldSlot) { super(oldSlot); } diff --git a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java index 9a67285682..abe2881e6e 100644 --- a/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java +++ b/rhino/src/main/java/org/mozilla/javascript/ScriptableObject.java @@ -350,7 +350,7 @@ public void put(Symbol key, Scriptable start, Object value) { @Override public void delete(String name) { checkNotSealed(name, 0); - slotMap.remove(name, 0); + slotMap.compute(name, 0, ScriptableObject::checkSlotRemoval); } /** @@ -363,14 +363,27 @@ public void delete(String name) { @Override public void delete(int index) { checkNotSealed(null, index); - slotMap.remove(null, index); + slotMap.compute(null, index, ScriptableObject::checkSlotRemoval); } /** Removes an object like the others, but using a Symbol as the key. */ @Override public void delete(Symbol key) { checkNotSealed(key, 0); - slotMap.remove(key, 0); + slotMap.compute(key, 0, ScriptableObject::checkSlotRemoval); + } + + private static Slot checkSlotRemoval(Object key, int index, Slot slot) { + if ((slot != null) && ((slot.getAttributes() & ScriptableObject.PERMANENT) != 0)) { + Context cx = Context.getContext(); + if (cx.isStrictMode()) { + throw ScriptRuntime.typeErrorById( + "msg.delete.property.with.configurable.false", key); + } + // This will cause removal to not happen + return slot; + } + return null; } /** @@ -551,13 +564,8 @@ public void setGetterOrSetter( AccessorSlot aSlot; if (isExtensible()) { - Slot slot = slotMap.modify(name, index, 0); - if (slot instanceof AccessorSlot) { - aSlot = (AccessorSlot) slot; - } else { - aSlot = new AccessorSlot(slot); - slotMap.replace(slot, aSlot); - } + // Create a new AccessorSlot, or cast it if it's already set + aSlot = slotMap.compute(name, index, ScriptableObject::ensureAccessorSlot); } else { Slot slot = slotMap.query(name, index); if (slot instanceof AccessorSlot) { @@ -648,15 +656,7 @@ protected boolean isGetterOrSetter(String name, int index, boolean setter) { void addLazilyInitializedValue(String name, int index, LazilyLoadedCtor init, int attributes) { if (name != null && index != 0) throw new IllegalArgumentException(name); checkNotSealed(name, index); - Slot slot = slotMap.modify(name, index, 0); - LazyLoadSlot lslot; - if (slot instanceof LazyLoadSlot) { - lslot = (LazyLoadSlot) slot; - } else { - lslot = new LazyLoadSlot(slot); - slotMap.replace(slot, lslot); - } - + LazyLoadSlot lslot = slotMap.compute(name, index, ScriptableObject::ensureLazySlot); lslot.setAttributes(attributes); lslot.value = init; } @@ -1537,15 +1537,7 @@ public void defineProperty( } } - Slot slot = slotMap.modify(propertyName, 0, 0); - AccessorSlot aSlot; - if (slot instanceof AccessorSlot) { - aSlot = (AccessorSlot) slot; - } else { - aSlot = new AccessorSlot(slot); - slotMap.replace(slot, aSlot); - } - + AccessorSlot aSlot = slotMap.compute(propertyName, 0, ScriptableObject::ensureAccessorSlot); aSlot.setAttributes(attributes); if (getterBox != null) { aSlot.getter = new AccessorSlot.MemberBoxGetter(getterBox); @@ -1613,59 +1605,67 @@ protected void defineOwnProperty( } } - Slot slot = slotMap.query(key, index); - boolean isNew = slot == null; - - if (checkValid) { - ScriptableObject current = slot == null ? null : slot.getPropertyDescriptor(cx, this); - checkPropertyChange(id, current, desc); - } - - boolean isAccessor = isAccessorDescriptor(desc); - final int attributes; - - if (slot == null) { - slot = slotMap.modify(key, index, 0); - attributes = applyDescriptorToAttributeBitset(DONTENUM | READONLY | PERMANENT, desc); - } else { - attributes = applyDescriptorToAttributeBitset(slot.getAttributes(), desc); - } - - if (isAccessor) { - AccessorSlot fslot; + // Do some complex stuff depending on whether or not the key + // already exists in a single hash map operation + slotMap.compute( + key, + index, + (k, ix, existing) -> { + if (checkValid) { + ScriptableObject current = + existing == null ? null : existing.getPropertyDescriptor(cx, this); + checkPropertyChange(id, current, desc); + } - if (slot instanceof AccessorSlot) { - fslot = (AccessorSlot) slot; - } else { - fslot = new AccessorSlot(slot); - slotMap.replace(slot, fslot); - } + Slot slot; + int attributes; + + if (existing == null) { + slot = new Slot(k, ix, 0); + attributes = + applyDescriptorToAttributeBitset( + DONTENUM | READONLY | PERMANENT, desc); + } else { + slot = existing; + attributes = + applyDescriptorToAttributeBitset(existing.getAttributes(), desc); + } - Object getter = getProperty(desc, "get"); - if (getter != NOT_FOUND) { - fslot.getter = new AccessorSlot.FunctionGetter(getter); - } - Object setter = getProperty(desc, "set"); - if (setter != NOT_FOUND) { - fslot.setter = new AccessorSlot.FunctionSetter(setter); - } + if (isAccessorDescriptor(desc)) { + AccessorSlot fslot; + if (slot instanceof AccessorSlot) { + fslot = (AccessorSlot) slot; + } else { + fslot = new AccessorSlot(slot); + slot = fslot; + } + Object getter = getProperty(desc, "get"); + if (getter != NOT_FOUND) { + fslot.getter = new AccessorSlot.FunctionGetter(getter); + } + Object setter = getProperty(desc, "set"); + if (setter != NOT_FOUND) { + fslot.setter = new AccessorSlot.FunctionSetter(setter); + } + fslot.value = Undefined.instance; + } else { + if (!slot.isValueSlot() && isDataDescriptor(desc)) { + // Replace a non-base slot with a regular slot + slot = new Slot(slot); + } + Object value = getProperty(desc, "value"); + if (value != NOT_FOUND) { + slot.value = value; + } else if (existing == null) { + // Ensure we don't get a zombie value if we have switched a lot + slot.value = Undefined.instance; + } + } - fslot.value = Undefined.instance; - fslot.setAttributes(attributes); - } else { - if (!slot.isValueSlot() && isDataDescriptor(desc)) { - Slot newSlot = new Slot(slot); - slotMap.replace(slot, newSlot); - slot = newSlot; - } - Object value = getProperty(desc, "value"); - if (value != NOT_FOUND) { - slot.value = value; - } else if (isNew) { - slot.value = Undefined.instance; - } - slot.setAttributes(attributes); - } + // After all that, whatever we return now ends up in the map + slot.setAttributes(attributes); + return slot; + }); } /** @@ -1684,19 +1684,10 @@ protected void defineOwnProperty( */ public void defineProperty( String name, Supplier getter, Consumer setter, int attributes) { - Slot slot = slotMap.modify(name, 0, attributes); - - LambdaSlot lSlot; - if (slot instanceof LambdaSlot) { - lSlot = (LambdaSlot) slot; - } else { - lSlot = new LambdaSlot(slot); - slotMap.replace(slot, lSlot); - } - - lSlot.getter = getter; - lSlot.setter = setter; - setAttributes(name, attributes); + LambdaSlot slot = slotMap.compute(name, 0, ScriptableObject::ensureLambdaSlot); + slot.setAttributes(attributes); + slot.getter = getter; + slot.setter = setter; } protected void checkPropertyDefinition(ScriptableObject desc) { @@ -2671,6 +2662,39 @@ Object[] getIds(boolean getNonEnumerable, boolean getSymbols) { return result; } + /* + * These are handy for changing slot types in one "compute" operation. + */ + private static AccessorSlot ensureAccessorSlot(Object name, int index, Slot existing) { + if (existing == null) { + return new AccessorSlot(name, index); + } else if (existing instanceof AccessorSlot) { + return (AccessorSlot) existing; + } else { + return new AccessorSlot(existing); + } + } + + private static LazyLoadSlot ensureLazySlot(Object name, int index, Slot existing) { + if (existing == null) { + return new LazyLoadSlot(name, index); + } else if (existing instanceof LazyLoadSlot) { + return (LazyLoadSlot) existing; + } else { + return new LazyLoadSlot(existing); + } + } + + private static LambdaSlot ensureLambdaSlot(Object name, int index, Slot existing) { + if (existing == null) { + return new LambdaSlot(name, index); + } else if (existing instanceof LambdaSlot) { + return (LambdaSlot) existing; + } else { + return new LambdaSlot(existing); + } + } + private void writeObject(ObjectOutputStream out) throws IOException { out.defaultWriteObject(); final long stamp = slotMap.readLock(); diff --git a/rhino/src/main/java/org/mozilla/javascript/Slot.java b/rhino/src/main/java/org/mozilla/javascript/Slot.java index ac0e445312..63d2904c76 100644 --- a/rhino/src/main/java/org/mozilla/javascript/Slot.java +++ b/rhino/src/main/java/org/mozilla/javascript/Slot.java @@ -19,9 +19,9 @@ public class Slot implements Serializable { transient Slot next; // next in hash table bucket transient Slot orderedNext; // next in linked list - Slot(Object name, int indexOrHash, int attributes) { + Slot(Object name, int index, int attributes) { this.name = name; - this.indexOrHash = indexOrHash; + this.indexOrHash = name == null ? index : name.hashCode(); this.attributes = (short) attributes; } diff --git a/rhino/src/main/java/org/mozilla/javascript/SlotMap.java b/rhino/src/main/java/org/mozilla/javascript/SlotMap.java index 1e942bdd8a..751478a8a2 100644 --- a/rhino/src/main/java/org/mozilla/javascript/SlotMap.java +++ b/rhino/src/main/java/org/mozilla/javascript/SlotMap.java @@ -17,6 +17,11 @@ */ public interface SlotMap extends Iterable { + @FunctionalInterface + public interface SlotComputer { + S compute(Object key, int index, Slot existing); + } + /** Return the size of the map. */ int size(); @@ -44,20 +49,19 @@ public interface SlotMap extends Iterable { */ Slot query(Object key, int index); - /** Replace "slot" with a new slot. This is used to change slot types. */ - void replace(Slot oldSlot, Slot newSlot); + /** + * Replace the value of key with the slot computed by the "compute" method. If "compute" throws + * an exception, make no change. If "compute" returns null, remove the mapping, otherwise, + * replace any existing mapping with the result of "compute", and create a new mapping if none + * exists. This is equivalant to the "compute" method on the Map interface, which simplifies + * code and is more efficient than making multiple calls to this interface. In order to allow + * use of multiple Slot subclasses, this function is templatized. + */ + S compute(Object key, int index, SlotComputer compute); /** * Insert a new slot to the map. Both "name" and "indexOrHash" must be populated. Note that * ScriptableObject generally adds slots via the "modify" method. */ void add(Slot newSlot); - - /** - * Remove the slot at either "key" or "index". - * - * @param key The key for the slot, which should be a String or a Symbol. - * @param index if key is zero, then this will be used as the key instead. - */ - void remove(Object key, int index); } diff --git a/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java b/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java index ea2ddd379d..63a53cb20b 100644 --- a/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java +++ b/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java @@ -58,8 +58,8 @@ public Slot modify(Object key, int index, int attributes) { } @Override - public void replace(Slot oldSlot, Slot newSlot) { - map.replace(oldSlot, newSlot); + public S compute(Object key, int index, SlotComputer c) { + return map.compute(key, index, c); } @Override @@ -73,11 +73,6 @@ public void add(Slot newSlot) { map.add(newSlot); } - @Override - public void remove(Object key, int index) { - map.remove(key, index); - } - @Override public Iterator iterator() { return map.iterator(); diff --git a/rhino/src/main/java/org/mozilla/javascript/ThreadSafeSlotMapContainer.java b/rhino/src/main/java/org/mozilla/javascript/ThreadSafeSlotMapContainer.java index 6d744b4fe5..e261702025 100644 --- a/rhino/src/main/java/org/mozilla/javascript/ThreadSafeSlotMapContainer.java +++ b/rhino/src/main/java/org/mozilla/javascript/ThreadSafeSlotMapContainer.java @@ -73,10 +73,10 @@ public Slot modify(Object key, int index, int attributes) { } @Override - public void replace(Slot oldSlot, Slot newSlot) { + public S compute(Object key, int index, SlotComputer c) { final long stamp = lock.writeLock(); try { - map.replace(oldSlot, newSlot); + return map.compute(key, index, c); } finally { lock.unlockWrite(stamp); } @@ -109,16 +109,6 @@ public void add(Slot newSlot) { } } - @Override - public void remove(Object key, int index) { - final long stamp = lock.writeLock(); - try { - map.remove(key, index); - } finally { - lock.unlockWrite(stamp); - } - } - /** * Take out a read lock on the slot map, if locking is implemented. The caller MUST call this * method before using the iterator, and MUST NOT call this method otherwise. diff --git a/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java b/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java index 07142b5057..b5701f454e 100644 --- a/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java +++ b/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java @@ -2,6 +2,7 @@ import static org.junit.Assert.*; +import java.lang.reflect.InvocationTargetException; import java.util.Arrays; import java.util.Collection; import java.util.HashSet; @@ -19,8 +20,9 @@ public class SlotMapTest { private final SlotMap map; public SlotMapTest(Class mapClass) - throws IllegalAccessException, InstantiationException { - this.map = mapClass.newInstance(); + throws IllegalAccessException, InstantiationException, IllegalArgumentException, + InvocationTargetException, NoSuchMethodException, SecurityException { + this.map = mapClass.getDeclaredConstructor().newInstance(); } @Parameterized.Parameters @@ -51,11 +53,11 @@ public void crudOneString() { assertEquals(1, map.size()); assertFalse(map.isEmpty()); Slot newSlot = new Slot(slot); - map.replace(slot, newSlot); + map.compute("foo", 0, (k, i, e) -> newSlot); Slot foundNewSlot = map.query("foo", 0); assertEquals("Testing", foundNewSlot.value); assertSame(foundNewSlot, newSlot); - map.remove("foo", 0); + map.compute("foo", 0, (k, ii, e) -> null); assertNull(map.query("foo", 0)); assertEquals(0, map.size()); assertTrue(map.isEmpty()); @@ -70,16 +72,82 @@ public void crudOneIndex() { assertEquals(1, map.size()); assertFalse(map.isEmpty()); Slot newSlot = new Slot(slot); - map.replace(slot, newSlot); + map.compute(null, 11, (k, i, e) -> newSlot); Slot foundNewSlot = map.query(null, 11); assertEquals("Testing", foundNewSlot.value); assertSame(foundNewSlot, newSlot); - map.remove(null, 11); + map.compute(null, 11, (k, ii, e) -> null); assertNull(map.query(null, 11)); assertEquals(0, map.size()); assertTrue(map.isEmpty()); } + @Test + public void computeReplaceSlot() { + Slot slot = map.modify("one", 0, 0); + slot.value = "foo"; + Slot newSlot = + map.compute( + "one", + 0, + (k, i, e) -> { + assertEquals(k, "one"); + assertEquals(i, 0); + assertNotNull(e); + assertEquals(e.value, "foo"); + Slot n = new Slot(e); + n.value = "bar"; + return n; + }); + assertEquals(newSlot.value, "bar"); + slot = map.query("one", 0); + assertEquals(slot.value, "bar"); + assertEquals(map.size(), 1); + } + + @Test + public void computeCreateNewSlot() { + Slot newSlot = + map.compute( + "one", + 0, + (k, i, e) -> { + assertEquals(k, "one"); + assertEquals(i, 0); + assertNull(e); + Slot n = new Slot(k, i, 0); + n.value = "bar"; + return n; + }); + assertNotNull(newSlot); + assertEquals(newSlot.value, "bar"); + Slot slot = map.query("one", 0); + assertNotNull(slot); + assertEquals(slot.value, "bar"); + assertEquals(map.size(), 1); + } + + @Test + public void computeRemoveSlot() { + Slot slot = map.modify("one", 0, 0); + slot.value = "foo"; + Slot newSlot = + map.compute( + "one", + 0, + (k, i, e) -> { + assertEquals(k, "one"); + assertEquals(i, 0); + assertNotNull(e); + assertEquals(e.value, "foo"); + return null; + }); + assertNull(newSlot); + slot = map.query("one", 0); + assertNull(slot); + assertEquals(map.size(), 0); + } + private static final int NUM_INDICES = 67; @Test @@ -96,33 +164,33 @@ public void manyKeysAndIndices() { assertFalse(map.isEmpty()); verifyIndicesAndKeys(); - // Randomly replace some stuff + // Randomly replace some slots for (int i = 0; i < 20; i++) { int ix = rand.nextInt(NUM_INDICES); Slot slot = map.query(null, ix); assertNotNull(slot); - Slot newSlot = new Slot(slot); - map.replace(slot, newSlot); + map.compute(null, ix, (k, j, e) -> new Slot(slot)); } for (int i = 0; i < 20; i++) { int ix = rand.nextInt(KEYS.length); Slot slot = map.query(KEYS[ix], 0); assertNotNull(slot); - Slot newSlot = new Slot(slot); - map.replace(slot, newSlot); + map.compute(KEYS[ix], 0, (k, j, e) -> new Slot(slot)); } verifyIndicesAndKeys(); + // Randomly remove slots -- which we do using compute because that's all + // that ScriptableObject needs. HashSet removedIds = new HashSet<>(); for (int i = 0; i < 20; i++) { int ix = rand.nextInt(NUM_INDICES); - map.remove(null, ix); + map.compute(null, ix, (k, ii, e) -> null); removedIds.add(ix); } HashSet removedKeys = new HashSet<>(); for (int i = 0; i < 20; i++) { int ix = rand.nextInt(NUM_INDICES); - map.remove(KEYS[ix], ix); + map.compute(KEYS[ix], ix, (k, ii, e) -> null); removedKeys.add(KEYS[ix]); }