From 7e5d9b946230daa0d4eace10e3027781b8ad3254 Mon Sep 17 00:00:00 2001 From: "duncan.macgregor" Date: Mon, 2 Dec 2024 11:55:28 +0000 Subject: [PATCH 1/3] Introduce `SingleEntrySlotMap` Refactor `SingleSlotMap` to be immutable. --- .../mozilla/javascript/SlotMapContainer.java | 118 ++++++++++++++++-- 1 file changed, 108 insertions(+), 10 deletions(-) diff --git a/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java b/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java index c3d0475f87..377ff67cd8 100644 --- a/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java +++ b/rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java @@ -8,6 +8,8 @@ import java.util.Collections; import java.util.Iterator; +import java.util.NoSuchElementException; +import java.util.Objects; /** * This class holds the various SlotMaps of various types, and knows how to atomically switch @@ -24,7 +26,7 @@ class SlotMapContainer extends SlotMapOwner implements SlotMap { static final int DEFAULT_SIZE = 10; - private static class EmptySlotMap implements SlotMap { + private static final class EmptySlotMap implements SlotMap { @Override public Iterator iterator() { @@ -43,9 +45,10 @@ public boolean isEmpty() { @Override public Slot modify(SlotMapOwner owner, Object key, int index, int attributes) { - var map = new EmbeddedSlotMap(); + var newSlot = new Slot(key, index, attributes); + var map = new SingleEntrySlotMap(newSlot); owner.setMap(map); - return map.modify(owner, key, index, attributes); + return newSlot; } @Override @@ -55,17 +58,112 @@ public Slot query(Object key, int index) { @Override public void add(SlotMapOwner owner, Slot newSlot) { - var map = new EmbeddedSlotMap(); - owner.setMap(map); - map.add(owner, newSlot); + if (newSlot != null) { + var map = new SingleEntrySlotMap(newSlot); + owner.setMap(map); + } } @Override public S compute( - SlotMapOwner owner, Object key, int index, SlotComputer compute) { - var map = new EmbeddedSlotMap(); - owner.setMap(map); - return map.compute(owner, key, index, compute); + SlotMapOwner owner, Object key, int index, SlotComputer c) { + var newSlot = c.compute(key, index, null); + if (newSlot != null) { + var map = new SingleEntrySlotMap(newSlot); + owner.setMap(map); + } + return newSlot; + } + } + + private static final class Iter implements Iterator { + private Slot next; + + Iter(Slot slot) { + next = slot; + } + + @Override + public boolean hasNext() { + return next != null; + } + + @Override + public Slot next() { + Slot ret = next; + if (ret == null) { + throw new NoSuchElementException(); + } + next = next.orderedNext; + return ret; + } + } + + static final class SingleEntrySlotMap implements SlotMap { + + SingleEntrySlotMap(Slot slot) { + assert (slot != null); + this.slot = slot; + } + + private final Slot slot; + + @Override + public Iterator iterator() { + return new Iter(slot); + } + + @Override + public int size() { + return 1; + } + + @Override + public boolean isEmpty() { + return false; + } + + @Override + public Slot modify(SlotMapOwner owner, Object key, int index, int attributes) { + final int indexOrHash = (key != null ? key.hashCode() : index); + + if (indexOrHash == slot.indexOrHash && Objects.equals(slot.name, key)) { + return slot; + } + Slot newSlot = new Slot(key, index, attributes); + add(owner, newSlot); + return newSlot; + } + + @Override + public Slot query(Object key, int index) { + final int indexOrHash = (key != null ? key.hashCode() : index); + + if (indexOrHash == slot.indexOrHash && Objects.equals(slot.name, key)) { + return slot; + } + return null; + } + + @Override + public void add(SlotMapOwner owner, Slot newSlot) { + if (owner == null) { + throw new IllegalStateException(); + } else { + var newMap = new EmbeddedSlotMap(); + owner.setMap(newMap); + newMap.add(owner, slot); + newMap.add(owner, newSlot); + } + } + + @Override + public S compute( + SlotMapOwner owner, Object key, int index, SlotComputer c) { + var newMap = new EmbeddedSlotMap(); + owner.setMap(newMap); + newMap.add(owner, slot); + return newMap.compute(owner, key, index, c); } } From 0ca210a9fae17bd73257b363db195c464e7606db Mon Sep 17 00:00:00 2001 From: "duncan.macgregor" Date: Wed, 11 Dec 2024 14:27:58 +0000 Subject: [PATCH 2/3] Put expected and actual assert arguments the right way round. --- rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java b/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java index e885f5d237..5407ddc11c 100644 --- a/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java +++ b/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java @@ -121,7 +121,7 @@ public void computeReplaceSlot() { assertEquals(newSlot.value, "bar"); slot = obj.getMap().query("one", 0); assertEquals(slot.value, "bar"); - assertEquals(obj.getMap().size(), 1); + assertEquals(1, obj.getMap().size()); } @Test @@ -145,7 +145,7 @@ public void computeCreateNewSlot() { Slot slot = obj.getMap().query("one", 0); assertNotNull(slot); assertEquals(slot.value, "bar"); - assertEquals(obj.getMap().size(), 1); + assertEquals(1, obj.getMap().size()); } @Test @@ -168,7 +168,7 @@ public void computeRemoveSlot() { assertNull(newSlot); slot = obj.getMap().query("one", 0); assertNull(slot); - assertEquals(obj.getMap().size(), 0); + assertEquals(0, obj.getMap().size()); } private static final int NUM_INDICES = 67; From 4f22b783433c901beec27a1de85a70ee2b8714d1 Mon Sep 17 00:00:00 2001 From: "duncan.macgregor" Date: Wed, 11 Dec 2024 14:44:35 +0000 Subject: [PATCH 3/3] Add `SingleSlotMap` to parameterised tests cases in `SlotMapTest`. Refactor test to match `SlotMapTest` to match `SingleEntrySlotMap`. --- .../org/mozilla/javascript/SlotMapTest.java | 85 ++++++++++++------- 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java b/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java index 5407ddc11c..b321d2ea1f 100644 --- a/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java +++ b/rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java @@ -2,12 +2,13 @@ import static org.junit.Assert.*; -import java.lang.reflect.InvocationTargetException; -import java.util.Arrays; import java.util.Collection; import java.util.HashSet; import java.util.Iterator; +import java.util.List; import java.util.Random; +import java.util.function.Supplier; +import java.util.stream.Collectors; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -30,35 +31,41 @@ public String getClassName() { private final ScriptableObject obj; - public SlotMapTest(Class mapClass) - throws IllegalAccessException, - InstantiationException, - IllegalArgumentException, - InvocationTargetException, - NoSuchMethodException, - SecurityException { - var map = mapClass.getDeclaredConstructor().newInstance(); - obj = new TestScriptableObject(); - obj.setMap(map); + // `SingleSlotMaps` will always have a slot, so we need to record + // the starting size of our slot map and take that into account + // when testing with one of those as the initial map. + private final int startingSize; + + public SlotMapTest(Supplier mapSupplier) { + this.obj = new TestScriptableObject(); + this.obj.setMap(mapSupplier.get()); + startingSize = this.obj.getMap().size(); } @Parameterized.Parameters public static Collection mapTypes() { - return Arrays.asList( - new Object[][] { - {EmbeddedSlotMap.class}, - {HashSlotMap.class}, - {SlotMapContainer.class}, - {ThreadSafeSlotMapContainer.class}, - }); + List> suppliers = + List.of( + () -> SlotMapContainer.EMPTY_SLOT_MAP, + () -> new SlotMapContainer.SingleEntrySlotMap(new Slot(new Object(), 0, 0)), + () -> new EmbeddedSlotMap(), + () -> new HashSlotMap(), + () -> new SlotMapContainer(), + () -> new ThreadSafeSlotMapContainer()); + return suppliers.stream().map(i -> new Object[] {i}).collect(Collectors.toList()); } @Test public void empty() { - assertEquals(0, obj.getMap().size()); - assertTrue(obj.getMap().isEmpty()); - assertNull(obj.getMap().query("notfound", 0)); - assertNull(obj.getMap().query(null, 123)); + if (startingSize == 0) { + assertEquals(0, obj.getMap().size()); + assertTrue(obj.getMap().isEmpty()); + assertNull(obj.getMap().query("notfound", 0)); + assertNull(obj.getMap().query(null, 123)); + } else { + assertEquals(startingSize, obj.getMap().size()); + assertFalse(obj.getMap().isEmpty()); + } } @Test @@ -67,7 +74,7 @@ public void crudOneString() { Slot slot = obj.getMap().modify(obj, "foo", 0, 0); assertNotNull(slot); slot.value = "Testing"; - assertEquals(1, obj.getMap().size()); + assertEquals(1 + startingSize, obj.getMap().size()); assertFalse(obj.getMap().isEmpty()); Slot newSlot = new Slot(slot); obj.getMap().compute(obj, "foo", 0, (k, i, e) -> newSlot); @@ -76,8 +83,12 @@ public void crudOneString() { assertSame(foundNewSlot, newSlot); obj.getMap().compute(obj, "foo", 0, (k, ii, e) -> null); assertNull(obj.getMap().query("foo", 0)); - assertEquals(0, obj.getMap().size()); - assertTrue(obj.getMap().isEmpty()); + assertEquals(0 + startingSize, obj.getMap().size()); + if (startingSize == 0) { + assertTrue(obj.getMap().isEmpty()); + } else { + assertFalse(obj.getMap().isEmpty()); + } } @Test @@ -86,7 +97,7 @@ public void crudOneIndex() { Slot slot = obj.getMap().modify(obj, null, 11, 0); assertNotNull(slot); slot.value = "Testing"; - assertEquals(1, obj.getMap().size()); + assertEquals(1 + startingSize, obj.getMap().size()); assertFalse(obj.getMap().isEmpty()); Slot newSlot = new Slot(slot); obj.getMap().compute(obj, null, 11, (k, i, e) -> newSlot); @@ -95,8 +106,12 @@ public void crudOneIndex() { assertSame(foundNewSlot, newSlot); obj.getMap().compute(obj, null, 11, (k, ii, e) -> null); assertNull(obj.getMap().query(null, 11)); - assertEquals(0, obj.getMap().size()); - assertTrue(obj.getMap().isEmpty()); + assertEquals(0 + startingSize, obj.getMap().size()); + if (startingSize == 0) { + assertTrue(obj.getMap().isEmpty()); + } else { + assertFalse(obj.getMap().isEmpty()); + } } @Test @@ -121,7 +136,7 @@ public void computeReplaceSlot() { assertEquals(newSlot.value, "bar"); slot = obj.getMap().query("one", 0); assertEquals(slot.value, "bar"); - assertEquals(1, obj.getMap().size()); + assertEquals(1 + startingSize, obj.getMap().size()); } @Test @@ -145,7 +160,7 @@ public void computeCreateNewSlot() { Slot slot = obj.getMap().query("one", 0); assertNotNull(slot); assertEquals(slot.value, "bar"); - assertEquals(1, obj.getMap().size()); + assertEquals(1 + startingSize, obj.getMap().size()); } @Test @@ -168,7 +183,7 @@ public void computeRemoveSlot() { assertNull(newSlot); slot = obj.getMap().query("one", 0); assertNull(slot); - assertEquals(0, obj.getMap().size()); + assertEquals(0 + startingSize, obj.getMap().size()); } private static final int NUM_INDICES = 67; @@ -183,7 +198,7 @@ public void manyKeysAndIndices() { Slot newSlot = obj.getMap().modify(obj, key, 0, 0); newSlot.value = key; } - assertEquals(KEYS.length + NUM_INDICES, obj.getMap().size()); + assertEquals(KEYS.length + NUM_INDICES + startingSize, obj.getMap().size()); assertFalse(obj.getMap().isEmpty()); verifyIndicesAndKeys(); @@ -244,6 +259,10 @@ private void verifyIndicesAndKeys() { } try { Iterator it = obj.getMap().iterator(); + for (int i = 0; i < startingSize; i++) { + // Skip initial slots + it.next(); + } for (int i = 0; i < NUM_INDICES; i++) { Slot slot = obj.getMap().query(null, i); assertNotNull(slot);