RFR(s): 8078463: optimize java/util/Map/Collisions.java (original) (raw)

Chris Hegarty chris.hegarty at oracle.com
Thu May 14 07:58:52 UTC 2015

On 14 May 2015, at 03:24, Martin Buchholz <martinrb at google.com> wrote:

Your changes look good,

Yes, this is a nice improvement.

but:

204 check(map.size() == i, "insertion: map expected size m%d != i%d", map.size(), i); many of those detail messages look like leftovers from a long debugging session. Here I would consider converting to a testng test (I ended up doing this a few times myself) and writing very simply, standardly, efficiently and readably assertEquals(map.size(), i);

Or could use jdk.testlibrary.Asserts.assertEquals, if you want to avoid spinning up the testng machinery, and generating yet another xml test report. If you are only interested in assertEquals.

only adding more breadcrumbs if it's non-obvious, which is generally not the case in this test.

Either if ok with me.

testMap already prints out keysdesc, so the test output is unambiguous.

On Wed, May 13, 2015 at 6:44 PM, Stuart Marks <stuart.marks at oracle.com> wrote:

Hi all,

Please review this change to optimize a test. Basically the test did string formatting for every assertion, but the string was thrown away if the assertion passed -- the most common case. The change is to do the string formatting only when an assertion fails and information needs to be printed out. Thanks to Andrey Zakharov for discovering and investigating this. Bug report: https://bugs.openjdk.java.net/browse/JDK-8078463 Webrev: http://cr.openjdk.java.net/~smarks/reviews/8078463/webrev.0/ On my (new, fast) laptop, with JVM options -Xcomp -XX:+DeoptimizeALot -client, the unmodified test takes about 21.4 seconds to run. The modified test takes only 12.3 seconds. Note that I have added several overloads of check() with different arguments. I tried an alternative, which is a varargs version of check(): static void check(boolean cond, String fmt, Object... args) { if (cond) { pass(); } else { fail(String.format(fmt, args)); } } This of course is much simpler code, but it took 14.2 seconds, about 15% slower than the proposed version. Is the simpler code worth the slowdown? I could go either way.

I’ve used the varargs version a number of times in my own tests. It is simple and easy to read. But if this 15% is important, then it could be worth the extra overloads. Either way I’m happy to this this change going in.

-Chris.

Thanks. s'marks

More information about the core-libs-dev mailing list