RFR(s): 8078463: optimize java/util/Map/Collisions.java
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 keys_desc, 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