add fix to identityhashmap, add one test class for it #212

lycoris106 · 2025-12-15T07:02:54Z

Resolves issue #209, and adds a corresponding test class IdentityHashMapTest.java

Fix

The fix follows the proposed fix in the issue, by removing the block:

if (i < deletedSlot && d >= deletedSlot &&
    traversalTable == IdentityHashMap.this.table) {
    int remaining = len - deletedSlot;
    Object[] newTable = new Object[remaining];
    System.arraycopy(tab, deletedSlot,
                     newTable, 0, remaining);
    traversalTable = newTable;
    index = 0;
}

The purpose of this block is to avoid revisiting keys after some keys are moved due to the closeDeletion process. However, this closeDeletion process would not affect nondex's version of iteration nextIndex() since it always loops through the whole table to find the key.

Also, another block in the remove method is removed to make our instrumentation simpler:

// If traversing a copy, remove in real table.
// We can skip gap-closure on copy.
if (tab != IdentityHashMap.this.table) {
    IdentityHashMap.this.remove(key);
    expectedModCount = modCount;
    return;
}

This block can also be safely removed because its purpose is to deal with the array-cutoff situation which only happens in the first block we removed.

Fix implementation

The implementation follow NonDex's practice of writing asm visitors to instrument bytecode. Just like addNextIndex, the addRemove method rewrites a new remove() for the iterator class. The original remove() is renamed into originalRemove (though we can choose to simply overwrite it since it's not used anymore).

Added Test Class

A test class IdentityHashMapTest is added into nondex-test module to specifically test the removeAll behavior. I chose to write a new test class since this test shouldn't be parameterized like MapTest. Since only IdentityHashMap's removeAll uses == to compare elements, the test wouldn't work for other maps.

The purpose of the test is to make sure the removeAll method succeeds without throwing ArrayIndexOutOfBoundsException due to the bug.

darko-marinov

Thanks for the contribution. My change requests are mostly minor.

darko-marinov · 2025-12-15T07:13:23Z

...x-instrumentation/src/main/java/edu/illinois/nondex/instr/IdentityHashMapShufflingAdder.java

    }

+    public void addRemove() {
+        MethodVisitor mv = super.visitMethod(Opcodes.ACC_PUBLIC, "remove", "()V", null, null);


How did you write this code?

I simply follow each line in the original remove method and convert each line into asm visitor by referring to existing NonDex code like addHasNext methods. The one part I had to find other reference and ask LLM for help is the Complex condition part, which requires some complex jump instructions. But overall the syntax are taken from other existing part in the file.

Ah, sorry we didn't tell you about ASMifier https://asm.ow2.io/faq.html#Q10

@EdwinIngJ did tell me about the tool, I used it to verify my instructions.

Have you tested your fix across multiple Java versions (for example, Java 8)? In some cases, the implementation can differ between versions.

Please double check java -version. If you're running on your VM, please email me the details so I can try out myself.

Sorry I was accidentally using the Oracle JDK 25 instead of OpenJDK 25. Now the build and test works fine. @EdwinIngJ Could you tell me more about the failure with Java 11? I tested in all above Java version including Java 11 and it worked fine.

My Java 11 JDK:

chihful2@fa25-cs527-009:~/forks/NonDex$ java --version openjdk 11.0.29 2025-10-21 OpenJDK Runtime Environment (build 11.0.29+7-post-Ubuntu-1ubuntu124.04) OpenJDK 64-Bit Server VM (build 11.0.29+7-post-Ubuntu-1ubuntu124.04, mixed mode, sharing)

I get the error running the integration tests:

[INFO] Building: comprehensive/pom.xml [INFO] run post-build script verify.groovy [INFO] The build exited with code 1. See /home/edwinji2/temp/NonDex/nondex-maven-plugin/target/it/comprehensive/build.log for details. [INFO] comprehensive/pom.xml ............................ FAILED (17.35 s)

[INFO] Running edu.illinois.nondex.core.MapTest [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.001 s <<< FAILURE! - in edu.illinois.nondex.core.MapTest [ERROR] testRemove[2](edu.illinois.nondex.core.MapTest) Time elapsed: 0 s <<< FAILURE! java.lang.AssertionError at edu.illinois.nondex.core.MapTest.testRemove(MapTest.java:141) [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.009 s <<< FAILURE! - in edu.illinois.nondex.core.MapTest [ERROR] testRemove[2](edu.illinois.nondex.core.MapTest) Time elapsed: 0 s <<< FAILURE! java.lang.AssertionError at edu.illinois.nondex.core.MapTest.testRemove(MapTest.java:141) [INFO] [INFO] Results: [INFO] [ERROR] Failures: [ERROR] edu.illinois.nondex.core.MapTest.testRemove[2](edu.illinois.nondex.core.MapTest) [ERROR] Run 1: MapTest.testRemove:141 [ERROR] Run 2: MapTest.testRemove:141 [ERROR] Run 3: MapTest.testRemove:141

Here is my Java version:

edwinji2@fa25-cs527-004:~/temp/NonDex$ java -version openjdk version "11.0.29" 2025-10-21 OpenJDK Runtime Environment (build 11.0.29+7-post-Ubuntu-1ubuntu124.04) OpenJDK 64-Bit Server VM (build 11.0.29+7-post-Ubuntu-1ubuntu124.04, mixed mode, sharing) edwinji2@fa25-cs527-004:~/temp/NonDex$ javac -version javac 11.0.29

I still couldn't reproduce the error myself. Running the integration tests with mvn clean install and running mvn clean verify -pl nondex-maven-plugin afterwards all passed for me. The MapTest in integration test should works exactly the same as the test in nondex-test module right? Is there a clean way to run the test possibly with different seeds like nondexRuns?

My integration tests result:

[INFO] --- invoker:3.9.1:integration-test (integration-test) @ nondex-maven-plugin --- [INFO] Building: failing-it/pom.xml [INFO] run post-build script verify.groovy [INFO] failing-it/pom.xml ............................... SUCCESS (11.36 s) [INFO] Building: simple-multimodule-it/pom.xml [INFO] run post-build script verify.groovy [INFO] simple-multimodule-it/pom.xml .................... SUCCESS (11.43 s) [INFO] Building: clean-it/pom.xml [INFO] run post-build script verify.groovy [INFO] clean-it/pom.xml ................................. SUCCESS (8.756 s) [INFO] Building: comprehensive/pom.xml [INFO] run post-build script verify.groovy [INFO] comprehensive/pom.xml ............................ SUCCESS (14.43 s) [INFO] Building: simple-it/pom.xml [INFO] run post-build script verify.groovy [INFO] simple-it/pom.xml ................................ SUCCESS (9.554 s)

Hmm, I also ran the same commands. I'm not aware of a clean way of specifying the seed in the test. But Github action was recently added to master. Try rebasing on top of master and let the Github action run and maybe that will provide more insight.

darko-marinov · 2025-12-15T07:14:18Z

nondex-test/src/test/java/edu/illinois/nondex/core/IdentityHashMapTest.java

+/*
+The MIT License (MIT)
+Copyright (c) 2015 Alex Gyori
+Copyright (c) 2022 Kaiyao Ke


Please add your name here.

The checkstyle enforces fixed header so I couldn't add my name in it. I added my name in comment above the test case instead, is this ok?

Fine as you did it, although we/you could have changed the Checkstyle config.

darko-marinov · 2025-12-15T07:14:54Z

nondex-test/src/test/java/edu/illinois/nondex/core/IdentityHashMapTest.java

+
+    @Test
+    public void testRemoveAllArrayIndexOutOfBounds() {
+        for (int i = 0; i < 10_000; i++) {


Do you really need 10K iterations? Would 100 or even fewer suffice?

I guess 1000 iteration is enough to make the original code fail 95% of the time. 100 is probably not enough for it to fail consistently. Should I make it 1000?

Why does it fail so rarely? Does it require some special condition among the three keys, e.g., that their System.identifyHashCode be equal? What if you put more than three keys?

It is indeed related to hash because the hash decides where the keys are placed in the underlying linear table. As the comment in JDK says:

// it must catch the rare case where an element already
// seen is swapped into a vacant slot that will be later
// traversed by this iterator. We cannot allow future
// next() calls to return it again. The likelihood of
// this occurring under 2/3 load factor is very slim, but
// when it does happen, we must make a copy of the rest of
// the table to use for the rest of the traversal. Since
// this can only happen when we are near the end of the table,
// even in these rare cases, this is not very expensive in
// time or space.

I believe this block (which is removed in my fix) only triggers when the deleted key is in later part of the table and also the other iterated keys are place in according positions. It is even rarer since I had only 3 elements, as it might be triggered more often when close to capacity of table.

Would it trigger more often if you have a map with a smaller capacity, e.g., new IdentityHashMap<>(3), or put more than three elements in?

You're right, I set the capacity to 4, and now the original removeAll fails 95% of the time with 100 iterations. I have committed the change.

Thanks for changing the initial capacity. Can we increase 100 iterations to 999 so that the failure goes from 95% to, hopefully, 99+%?

darko-marinov · 2025-12-15T07:15:31Z

nondex-test/src/test/java/edu/illinois/nondex/core/IdentityHashMapTest.java

+            map.put(k2, 2);
+            map.put(k3, 3);
+
+            map.keySet().removeAll(Collections.singleton(k2));


Is there some assertion to check (e.g., that the size of map is two after this), or are you simply checking that this doesn't throw an exception?

Yes I am simply checking that this doesn't throw an exception. I consider it redundant to add additional assertions because most map behaviors are already checked by tests in MapTest, etc.

Okay, fine to not have an assertion.

darko-marinov · 2025-12-15T07:15:40Z

nondex-test/src/test/java/edu/illinois/nondex/core/IdentityHashMapTest.java

+            map.keySet().removeAll(Collections.singleton(k2));
+        }
+    }
+}


Please add newline at the end.

darko-marinov · 2025-12-15T07:17:03Z

@EdwinIngJ and @cperkkk could you take a quick look at this PR?

lycoris106 · 2025-12-16T03:59:41Z

Pushed the commit fixing access problem in Java 8. Tested the fix on Java 8, Java 17, Java 21.

darko-marinov · 2025-12-17T03:26:54Z

We're getting close to the deadline, so please squash your four commit into one, and I can accept the PR for now. We can (partially) revert it later if it does break something.

darko-marinov · 2025-12-17T03:34:17Z

Note that all the checks have passed for Java 8/11/17/21/25 on Maven and Java 8/11/17/21 on Gradle.

add author comment and newline minimize table size to increase failing probability for removeAll Fixed incompatibility with Java 8 by inlining outer class methods increase test iteration number

lycoris106 · 2025-12-17T03:46:19Z

Squashed all commits. All above problems seem to have been resolved. Feel free to contact me if it does break something afterwards!

darko-marinov

Let's accept for now (and revert later if needed). Thank you @lycoris106 and @EdwinIngJ for working on this until the last hour!

darko-marinov requested changes Dec 15, 2025

View reviewed changes

lycoris106 force-pushed the identity-hashmap-removeall branch from c29f8a8 to be3ff10 Compare December 17, 2025 00:30

add fix to identityhashmap, add one test class for it

90f3d5e

add author comment and newline minimize table size to increase failing probability for removeAll Fixed incompatibility with Java 8 by inlining outer class methods increase test iteration number

lycoris106 force-pushed the identity-hashmap-removeall branch from be3ff10 to 90f3d5e Compare December 17, 2025 03:42

darko-marinov approved these changes Dec 17, 2025

View reviewed changes

darko-marinov merged commit a05b4cb into TestingResearchIllinois:master Dec 17, 2025

add fix to identityhashmap, add one test class for it #212

add fix to identityhashmap, add one test class for it #212

Uh oh!

Conversation

lycoris106 commented Dec 15, 2025

Fix

Fix implementation

Added Test Class

Uh oh!

darko-marinov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lycoris106 Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lycoris106 Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lycoris106 Dec 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lycoris106 Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darko-marinov commented Dec 15, 2025

Uh oh!

lycoris106 commented Dec 16, 2025

Uh oh!

darko-marinov commented Dec 17, 2025

Uh oh!

darko-marinov commented Dec 17, 2025

Uh oh!

lycoris106 Dec 15, 2025 •

edited

Loading

lycoris106 Dec 16, 2025 •

edited

Loading

lycoris106 Dec 16, 2025 •

edited

Loading

lycoris106 Dec 15, 2025 •

edited

Loading

lycoris106 commented Dec 17, 2025 •

edited

Loading