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

[Stuart Marks]

Hi Martin, thanks for taking a look.

It probably would be a good idea to convert this test (and a whole bunch of 
others) to Test-NG. However, that's more change than I want to bite off at this 
point, so I'd prefer to stick with my change as it stands right now.

s'marks

On 5/13/15 7:24 PM, Martin Buchholz wrote:
> Your changes look good, 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);
>
> only adding more breadcrumbs if it's non-obvious, which is generally not the 
> case in this test.
>
> testMap already prints out keys_desc, so the test output is unambiguous.
>
> On Wed, May 13, 2015 at 6:44 PM, Stuart Marks <stuart.marks at oracle.com 
> <mailto: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/
>     <http://cr.openjdk.java.net/%7Esmarks/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.
>
>     Thanks.
>
>     s'marks
>
>
>