Hi Amy, As an exercise it might useful to modify ArrayList and test with the existing benchmark. One could follow up with ArrayList changes as a separate issue. If your investigations show no regressions then we have good confidence the current patch is ok.
Another useful learning exercise is to rewrite the ArrayList range check benchmark to JMH. I am piling more work on you :-) but it’s a good for learning. The important aspect when testing for regression on range checks is to ensure the cost is not dwarfed by the other work. What array size/range values did you use in the test below? Paul. > On 19 Jan 2017, at 05:11, Amy Lu <[email protected]> wrote: > > No, no change to ArrayList, sorry for the confusion. I just thought > ArrayList/RangeCheckMicroBenchmark.java can also give some data for reference > to the Arrays change. > > So I tried a modified ArraysEquals JMH test, the test is originally for > equals(char[] a, char[] a2) and I changed it to perform > equals(char[] a, int aFromIndex, int aToIndex, > char[] b, int bFromIndex, int bToIndex) > which is changed in this patch. > > No performance regression. > > without 8146668 > > Benchmark Mode Cnt > Score Error Units > ArraysEqualsFromTo.testCharArrayEqualsFalseBeginning thrpt 200 > 117773032.914 ? 837866.545 ops/s > ArraysEqualsFromTo.testCharArrayEqualsFalseEnd thrpt 200 > 68162413.018 ? 252693.599 ops/s > ArraysEqualsFromTo.testCharArrayEqualsFalseMid thrpt 200 > 87845305.867 ? 434145.358 ops/s > ArraysEqualsFromTo.testCharArrayEqualsTrue thrpt 200 > 57665790.396 ? 1144777.344 ops/s > > With 8146668 > > Benchmark Mode Cnt > Score Error Units > ArraysEqualsFromTo.testCharArrayEqualsFalseBeginning thrpt 200 > 118754451.736 ? 395812.217 ops/s > ArraysEqualsFromTo.testCharArrayEqualsFalseEnd thrpt 200 > 68263418.139 ? 161477.719 ops/s > ArraysEqualsFromTo.testCharArrayEqualsFalseMid thrpt 200 > 87429210.156 ? 483026.299 ops/s > ArraysEqualsFromTo.testCharArrayEqualsTrue thrpt 200 > 57694918.966 ? 1163825.417 ops/s > > > Thanks, > Amy > > On 1/18/17 9:36 AM, Paul Sandoz wrote: >> Hi Amy, >> >> RangeCheckMicroBenchmark.java measures access for ArrayList. Did you update >> ArrayList? (i don’t see it in your current patch). >> >> Paul. >> >>> On 16 Jan 2017, at 01:41, Amy Lu <[email protected]> wrote: >>> >>> Thank you Paul and Martin for your review. >>> >>> On 1/13/17 12:52 AM, Paul Sandoz wrote: >>>> HI Amy, >>>> >>>> Overall this looks very good, well done. >>>> >>>> >>>> At this point we are down to two things: >>>> >>>> 1) should we preserve exception messages? >>>> >>>> 2) due diligence on the performance. >>>> >>>> >>>> On 1) my preference is that uniform (and informative) messages are better >>>> for IndexOutOfBounds and subtypes, and defining that in addition to the >>>> checks in one place is very valuable. In some sense that does change some >>>> behavioural compatibility and i think because of that pushing in 10 (when >>>> the repos) open with a CCC would be more preferable. >>> Got it. Will wait for 10. >>>> On 2) this is a valuable exercise to perform (either using an existing >>>> test and/or writing JMH benchmarks). I don’t expect any major problems. >>>> Care was taken to ensure the uncommon exception processing path will >>>> optimize away for the common case. The BiFunction parameter passed as the >>>> last argument is always a constant, so should fold away from the hot path. >>> Tested with java/util/ArrayList/RangeCheckMicroBenchmark.java and >>> java/nio/Buffer/SwapMicroBenchmark.java, no performance regression observed. >>> >>> java/util/ArrayList/RangeCheckMicroBenchmark.java >>> >>> without 8146668 >>> -------------------- >>> get 26 1 >>> set 40.6 1.5707 >>> get/set 85.7 3.3002 >>> add/remove at end 337.5 12.9197 >>> subList get 25 0.9747 >>> subList set 59.9 2.3 >>> subList get/set 92.2 3.5414 >>> subList add/remove at end 429.8 16.45 >>> >>> with 8146668 >>> -------------------- >>> get 26 1 >>> set 39.1 1.5188 >>> get/set 86.8 3.3363 >>> add/remove at end 335.5 12.8549 >>> subList get 24.8 0.9715 >>> subList set 59.2 2.2728 >>> subList get/set 92.2 3.5473 >>> subList add/remove at end 427.7 16.3789 >>> >>> java/nio/Buffer/SwapMicroBenchmark.java >>> >>> without 8146668 >>> -------------------- >>> swap char LITTLE_ENDIAN 11 1 >>> swap short LITTLE_ENDIAN 10.2 0.9895 >>> swap int LITTLE_ENDIAN 5 0.4961 >>> swap long LITTLE_ENDIAN 3 0.3345 >>> >>> with 8146668 >>> -------------------- >>> swap char LITTLE_ENDIAN 10.1 1 >>> swap short LITTLE_ENDIAN 10.1 0.9995 >>> swap int LITTLE_ENDIAN 5 0.5005 >>> swap long LITTLE_ENDIAN 3 0.3327 >>> >>> Thanks, >>> Amy >>>> Paul. >>>> >>>> >>>>> On 11 Jan 2017, at 18:10, Amy Lu <[email protected]> wrote: >>>>> >>>>> 8135248 and 8155794 introduced utility methods for checking indexes and >>>>> ranges. Existing code with custom checkIndex/checkRange can be updated to >>>>> use these methods. Please review the patch for this purpose: >>>>> >>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8146668 >>>>> webrev: http://cr.openjdk.java.net/~amlu/8146668/webrev.01 >>>>> >>>>> The type of exception thrown are preserved. Custom checkIndex/checkRange >>>>> functions that throw IOOBE are now using ‘check’ utility methods provided >>>>> by java.lang.Objects (which also throws IOOBE), functions that throw >>>>> other exceptions use jdk.internal.util.Preconditions to preserve >>>>> exception types, with the help of new BiFunction vars. >>>>> >>>>> I'd like to get this in JDK 9 if it's not too late, otherwise, JDK 10. >>>>> >>>>> Thanks, >>>>> Amy >
