Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Single entry slot map #1784

Merged
merged 3 commits into from
Jan 26, 2025
Merged

Single entry slot map #1784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7e5d9b9
Introduce `SingleEntrySlotMap`
aardvark179 Dec 2, 2024
0ca210a
Put expected and actual assert arguments the right way round.
aardvark179 Dec 11, 2024
4f22b78
Add `SingleSlotMap` to parameterised tests cases in `SlotMapTest`.
aardvark179 Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 108 additions & 10 deletions rhino/src/main/java/org/mozilla/javascript/SlotMapContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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<Slot> iterator() {
Expand All @@ -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
Expand All @@ -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 extends Slot> S compute(
SlotMapOwner owner, Object key, int index, SlotComputer<S> compute) {
var map = new EmbeddedSlotMap();
owner.setMap(map);
return map.compute(owner, key, index, compute);
SlotMapOwner owner, Object key, int index, SlotComputer<S> 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<Slot> {
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<Slot> 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 extends Slot> S compute(
SlotMapOwner owner, Object key, int index, SlotComputer<S> c) {
var newMap = new EmbeddedSlotMap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, "compute" is also used to replace an existing slot, rather than add a new one. I don't know how much that happens in actual situations, but there are plenty of places in the codebase where it happens. With this implementation, in that case we'll upgrade to a "real" EmbeddedSlotMap, but then it still may have a size of one.

Would it complicate things a lot to support that use case? If not, should we try it? I would also be open to the idea that this would be a premature optimization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose to always promote there because I figured that a map which is mutated is likely to continue to be mutated, and it made it easier to think about the multithreaded cases.

Removing a slot also causes the map to be promoted as that maintains a strict type lattice of slot map types (we never demote a map). I tried to support that sort of thing initially but it made the code much more complex and was rarely triggered in my testing so I went for the simpler approach seen here.

Ultimately I think we should separate values and slot descriptors, and make the latter immutable. That would open up some good optimization opportunities in both the interpreter and compiler, and would lI think save quite a bit of memory.

owner.setMap(newMap);
newMap.add(owner, slot);
return newMap.compute(owner, key, index, c);
}
}

Expand Down
85 changes: 52 additions & 33 deletions rhino/src/test/java/org/mozilla/javascript/SlotMapTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -30,35 +31,41 @@ public String getClassName() {

private final ScriptableObject obj;

public SlotMapTest(Class<SlotMap> 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<SlotMap> mapSupplier) {
this.obj = new TestScriptableObject();
this.obj.setMap(mapSupplier.get());
startingSize = this.obj.getMap().size();
}

@Parameterized.Parameters
public static Collection<Object[]> mapTypes() {
return Arrays.asList(
new Object[][] {
{EmbeddedSlotMap.class},
{HashSlotMap.class},
{SlotMapContainer.class},
{ThreadSafeSlotMapContainer.class},
});
List<Supplier<SlotMap>> 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
Expand All @@ -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);
Expand All @@ -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
Expand All @@ -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);
Expand All @@ -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
Expand All @@ -121,7 +136,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 + startingSize, obj.getMap().size());
}

@Test
Expand All @@ -145,7 +160,7 @@ public void computeCreateNewSlot() {
Slot slot = obj.getMap().query("one", 0);
assertNotNull(slot);
assertEquals(slot.value, "bar");
assertEquals(obj.getMap().size(), 1);
assertEquals(1 + startingSize, obj.getMap().size());
}

@Test
Expand All @@ -168,7 +183,7 @@ public void computeRemoveSlot() {
assertNull(newSlot);
slot = obj.getMap().query("one", 0);
assertNull(slot);
assertEquals(obj.getMap().size(), 0);
assertEquals(0 + startingSize, obj.getMap().size());
}

private static final int NUM_INDICES = 67;
Expand All @@ -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();

Expand Down Expand Up @@ -244,6 +259,10 @@ private void verifyIndicesAndKeys() {
}
try {
Iterator<Slot> 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);
Expand Down
Loading