Skip to content

Commit

Permalink
Fix hash size calculations counting cleared keys
Browse files Browse the repository at this point in the history
When the hash part of the table is resized, we count the number of still
active nodes and create an array to the nearest power of two. However,
our handling here was broken - we counted nodes with a nil key* and not
*value*.

If a table had several keys added to it, and then removed, and then a
new set of keys added, this would cause both the old and new keys to be
counted, causing the new hash array to be much larger than needed.

While the old keys are not copied across (so they're not leaked as such),
if we continue to add/remove unique keys, eventually the new map will
also be saturated, and then resized to be even bigger again! This means
that the length of the hash part can grow exponentially, even if the
number of keys stays constant.

Embarassing that this has gone un-noticed for 6 years!

Fixes cc-tweaked/CC-Tweaked#1665
  • Loading branch information
SquidDev committed Jan 2, 2024
1 parent a6e3033 commit 2513ce7
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 7 deletions.
9 changes: 2 additions & 7 deletions src/main/java/org/squiddev/cobalt/LuaTable.java
Original file line number Diff line number Diff line change
Expand Up @@ -468,10 +468,6 @@ private void resize(int newArraySize, int newHashSize, boolean modeChange) {
int oldArraySize = array.length;
int oldHashSize = nodes.length;

if (newArraySize != 0 && newHashSize != 0 && newArraySize == oldArraySize && newHashSize == oldHashSize && !modeChange) {
throw new IllegalStateException("Attempting to resize with no change");
}

// Array part must grow
if (newArraySize > oldArraySize) {
array = setArrayVector(array, newArraySize, modeChange, weakValues);
Expand Down Expand Up @@ -521,9 +517,8 @@ private void rehash(LuaValue extraKey, boolean mode) {
int i = nodes.length;
while (--i >= 0) {
Node node = nodes[i];
LuaValue key = node.key();
if (!key.isNil()) {
arrayCount += countInt(key, nums);
if (!node.value().isNil()) {
arrayCount += countInt(node.key(), nums);
totalCount++;
}
}
Expand Down
18 changes: 18 additions & 0 deletions src/test/java/org/squiddev/cobalt/table/TableHashTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -248,4 +248,22 @@ public void testNext() throws LuaError, UnwindThrowable {
assertEquals(ValueFactory.valueOf("bbb"), t.next(ValueFactory.valueOf("aa")).arg(2));
assertEquals(Constants.NIL, t.next(ValueFactory.valueOf("bb")));
}

@Test
public void testShrink() {
LuaTable t = new LuaTable();

// Setting our initial values creates the right hash part.
for (int i = 0; i < 16; i++) t.rawset("key_" + i, Constants.TRUE);
assertEquals(16, TableOperations.getHashLength(t));

// We then clear these values, and check the hash part is unchanged.
for (int i = 0; i < 16; i++) t.rawset("key_" + i, Constants.NIL);
assertEquals(16, TableOperations.getHashLength(t));

// We then repopulate with new objects, and assert the hash part has been recreated
// with the correct size.
for (int i = 0; i < 8; i++) t.rawset("new_key_" + i, Constants.TRUE);
assertEquals(8, TableOperations.getHashLength(t));
}
}

0 comments on commit 2513ce7

Please sign in to comment.