On 14 May 2015, at 03:24, Martin Buchholz <marti...@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.ma...@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 >> >> >>
