Assert.assertArrayEquals terrible performance

[amichair]

Using Assert.assertArrayEquals for comparing e.g. a byte array (in JUnit 4.10, OpenJDK 1.7.0_03 on Ubuntu) has unacceptably slow performance, between one and two orders of magnitude slower than a trivial implementation of the method with the exact same contract. The difference is likely due to the fact that it uses reflection (Array.get) to get every single element of the array, boxing and all, do some irrelevant checks on the element (e.g. is it an array) etc. Instantiating a new ExactComparisonCriteria for each call even though it's a stateless class that can be reused is unnecessary as well (though likely not as bad for performance, since it's once per call, whereas the reflection stuff is once per array element per call).

In one particular test of mine that uses this assertion on byte arrays, using the built-in assertArrayEquals it takes over 15 seconds to run, whereas using a custom version of the method that does the null and length checks and just compares the arrays using a trivial loop, it runs in far less than a second. The difference is huge!

This naturally holds for all primitive arrays. float/double might be a tad longer to implement due to delta checks, but it should still be easy enough and far faster than the generic reflective implementation.

[awulder]

I have tried to reproduce the performance issue but I can't reproduce it.
Can you please add an example of a test that causes this performance issue?

[amichair]

I don't see an option to attache a file here, so I've pasted it inline below.

A typical output of running the test via ant using JUnit's assertArrayEquals:

[junit] Running ArrayEqualsAssertionPerformanceTest
[junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 6.575 sec

and using the provided assertArrayEqualsNative (just swap the commented line):

[junit] Running ArrayEqualsAssertionPerformanceTest
[junit] Tests run: 1, Failures: 0, Errors: 0, Time elapsed: 0.067 sec

Repeating the tests multiple times slightly varies the results, but they remain in the order of x100 speed difference.

Some Java env variables (as shown by ant -v when running the tests):
java.runtime.name=OpenJDK Runtime Environment
java.runtime.version=1.7.0_03-b147
java.vm.name=OpenJDK 64-Bit Server VM
java.vm.version=22.0-b10

Running on Kubuntu 12.04 with kernel 3.5.0-030500-generic from the Ubuntu mainline kernel PPA.

import java.io.IOException;
import static org.junit.Assert.*;
import org.junit.Test;

public class ArrayEqualsAssertionPerformanceTest {

    static public void assertArrayEqualsNative(String message, byte[] expected, byte[] actual) {
        if (expected == null || actual == null) {
            assertEquals(message, expected, actual);
        } else {
            assertEquals(message, expected.length, actual.length);
            boolean equal = true;
            for (int i = 0; i < expected.length; i++) {
                if (expected[i] != actual[i]) {
                    equal = false;
                    break;
                }
            }
            assertTrue(message, equal);
        }
    }


    @Test
    public void testArrayEqualsAssertionPerformance() throws IOException {
        for (int len = 0; len < 8192; len++) {
            byte[] a = new byte[len];
            byte[] b = new byte[len];
            // swap the commented line below to test the two different implementations
            assertArrayEquals("equal arrays - junit", a, b);
            //assertArrayEqualsNative("equal arrays - native", a, b);
        }
    }
}

[awulder]

With your code I can reproduce the perfomance issue and it's huge. In my case our solution is about 200x faster. However, I do understand why the JUnit team uses reflection in this case. I think it's up to them to decide if they want to refactor the code.

[amichair]

I'm glad to hear it's reproducible. Note that my alternate implementation is by no means the suggested solution - it doesn't throw the same assertions with the same messages as the original implementation, for one. It's just a quick example I put together to demonstrate the performance issue. If you already have an efficient, tested and contract-preserving alternative implementation for the solution, I suppose there's no harm in posting it here ;-)

I do hope this gets fixed in JUnit itself. I understand the convenience of unifying near-duplicate code, and have suffered from the primitive array handling duplication that Java requires (even a quick glimpse of java.util.Arrays is quite nauseating), but alas, that is a limitation of the language. But in such core parts of the Java platform as java.util and JUnit, I think the performance price of the reflection workaround is just too steep, and we're better off living with a hurting sense of aesthetics but reasonably performing code...

That's my opinion, anyway :-)

[mattj256]

I have been experimenting with this. The low-hanging fruit are arrays of primitive types. In my tests our code is around 70-100x slower than baseline, and I have a working version that's "only" 3-5x slower than baseline. The trade-off is that ExactComparisonCriteria and InexactComparisonCriteria would need an overloaded method for each type of primitive array.

Is that something people want? Some of the logic can be factored out, like the null check and the array length check. But I don't know any way to make the main loop efficient other than to copy-paste it into each overloaded method.

[kcooney]

@mattj256 I think the copy-paste approach would be reasonable here. Would you mind using https://code.google.com/p/caliper/ to get before and after performance results for one of the modified methods?

[mattj256]

That's fine. I'll add it to my queue.

[eamonnmcmanus]

Let's not forget that there are already all the necessary overloaded forms of Arrays.equals. The loop in assertArrayEqualsNative can be reduced to boolean equal = Arrays.equals(expected, actual).

[kcooney]

@eamonnmcmanus are you willing to send us a pull request to make that change?

[eamonnmcmanus]

Sure.

[kcooney]

This was fixed in 4.12

[amichair]

Better late than never ;-)

Thanks to everyone involved in the fix!

For reference, the fix and its discussion is in #918, which I don't see linked here. btw I like the solution - minimal code for most of the performance gain - good tradeoff imho.