RFR 8081027: Create a common test to check adequacy of initial size of static HashMap/ArrayList fields
Hello! This is in some way continuation of JDK-8080535 (Expected size of Character.UnicodeBlock.map is not optimal). A new utility class OptimalCapacity was added to jdk.testlibrary package. The test NonOptimalMapSize.java that had been added with JDK-8080535, was rewritten with use of this new class. A couple more tests were added, which utilize methods of OptimalCapacity for checking sizes of ArrayList and IdentityHashMap static variables. Optimization of initial sizes of two more variables saves us one reallocation during java start-time and a few more bytes of memory. Would you please help review this fix? BUGURL: https://bugs.openjdk.java.net/browse/JDK-8081027 WEBREV: http://cr.openjdk.java.net/~igerasim/8081027/00/webrev/ Sincerely yours, Ivan
Thanks. Your methods like ofArrayList declare that they throw ReflectiveOperationException, but also have a try/catch to prevent that from ever happening. The assertions e.g. OptimalCapacity.ofArrayList(XClass.class, "theList", N) could look more like assertions, e.g. OptimalCapacity.assertOptimallySized(Class<?> clazz, String fieldName, int initialCapacity) getStorage returns the actual internal array, but you only ever need the length. Maybe I would have a single method static int internalArraySize(Object x) { ... } "enteties" is misspelled. For ArrayLists, I would have been happy enough just testing that we have 100% utilization, i.e. size of array is the same as size of the List, without checking the initial capacity. On Mon, May 25, 2015 at 4:07 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com> wrote:
Hello!
This is in some way continuation of JDK-8080535 (Expected size of Character.UnicodeBlock.map is not optimal).
A new utility class OptimalCapacity was added to jdk.testlibrary package. The test NonOptimalMapSize.java that had been added with JDK-8080535, was rewritten with use of this new class. A couple more tests were added, which utilize methods of OptimalCapacity for checking sizes of ArrayList and IdentityHashMap static variables.
Optimization of initial sizes of two more variables saves us one reallocation during java start-time and a few more bytes of memory.
Would you please help review this fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8081027 WEBREV: http://cr.openjdk.java.net/~igerasim/8081027/00/webrev/
Sincerely yours, Ivan
Thank you Martin for looking at this! On 27.05.2015 17:30, Martin Buchholz wrote:
Thanks.
Your methods like ofArrayList declare that they throw ReflectiveOperationException, but also have a try/catch to prevent that from ever happening.
Fixed!
The assertions e.g. OptimalCapacity.ofArrayList(XClass.class, "theList", N) could look more like assertions, e.g. OptimalCapacity.assertOptimallySized(Class<?> clazz, String fieldName, int initialCapacity)
Good naming is one of the most difficult part of coding IMO. The chosen names were meant to be read literally: "optimal capacity of ArrayList", etc. I agree, 'assert' would tell more about these methods' intention, but I'm not sure how to include it in the names. Maybe rename the class to AssertOptimalCapacity?
getStorage returns the actual internal array, but you only ever need the length. Maybe I would have a single method
static int internalArraySize(Object x) { ... }
Yes. I was going to compare the references, but finished comparing only sizes. Changed the method to internalArraySize(), as you suggested.
"enteties" is misspelled.
Fixed.
For ArrayLists, I would have been happy enough just testing that we have 100% utilization, i.e. size of array is the same as size of the List, without checking the initial capacity.
But then the test wouldn't have caught the "bug" in src/java.base/share/classes/sun/security/ssl/ExtensionType.java The ArrayList was pre-sized to 9, and after reallocation the capacity happened to become (9 + 9/2) = 13, which by coincidence is the final size of the List. Please have a look at the updated webrev: http://cr.openjdk.java.net/~igerasim/8081027/01/webrev/ Sincerely yours, Ivan
On Mon, May 25, 2015 at 4:07 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com <mailto:ivan.gerasimov@oracle.com>> wrote:
Hello!
This is in some way continuation of JDK-8080535 (Expected size of Character.UnicodeBlock.map is not optimal).
A new utility class OptimalCapacity was added to jdk.testlibrary package. The test NonOptimalMapSize.java that had been added with JDK-8080535, was rewritten with use of this new class. A couple more tests were added, which utilize methods of OptimalCapacity for checking sizes of ArrayList and IdentityHashMap static variables.
Optimization of initial sizes of two more variables saves us one reallocation during java start-time and a few more bytes of memory.
Would you please help review this fix?
BUGURL: https://bugs.openjdk.java.net/browse/JDK-8081027 WEBREV: http://cr.openjdk.java.net/~igerasim/8081027/00/webrev/ <http://cr.openjdk.java.net/%7Eigerasim/8081027/00/webrev/>
Sincerely yours, Ivan
Approved. On Thu, May 28, 2015 at 3:43 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com> wrote: Good naming is one of the most difficult part of coding IMO.
The chosen names were meant to be read literally: "optimal capacity of ArrayList", etc. I agree, 'assert' would tell more about these methods' intention, but I'm not sure how to include it in the names. Maybe rename the class to AssertOptimalCapacity?
Assertions are a class of names we are used to importing statically. Junit and Testng both have collections of static methods named assertXXX.
For ArrayLists, I would have been happy enough just testing that we have 100% utilization, i.e. size of array is the same as size of the List, without checking the initial capacity.
But then the test wouldn't have caught the "bug" in src/java.base/share/classes/sun/security/ssl/ExtensionType.java The ArrayList was pre-sized to 9, and after reallocation the capacity happened to become (9 + 9/2) = 13, which by coincidence is the final size of the List.
It's a tradeoff of maintenance burden vs performance. Note also that oversize is a much bigger performance bug than undersize because you pay for it for the entire process, not just startup.
Thank you Martin for review! On 29.05.2015 3:03, Martin Buchholz wrote:
Approved.
On Thu, May 28, 2015 at 3:43 PM, Ivan Gerasimov <ivan.gerasimov@oracle.com <mailto:ivan.gerasimov@oracle.com>> wrote:
Good naming is one of the most difficult part of coding IMO. The chosen names were meant to be read literally: "optimal capacity of ArrayList", etc. I agree, 'assert' would tell more about these methods' intention, but I'm not sure how to include it in the names. Maybe rename the class to AssertOptimalCapacity?
Assertions are a class of names we are used to importing statically. Junit and Testng both have collections of static methods named assertXXX.
For ArrayLists, I would have been happy enough just testing that we have 100% utilization, i.e. size of array is the same as size of the List, without checking the initial capacity.
But then the test wouldn't have caught the "bug" in src/java.base/share/classes/sun/security/ssl/ExtensionType.java The ArrayList was pre-sized to 9, and after reallocation the capacity happened to become (9 + 9/2) = 13, which by coincidence is the final size of the List.
It's a tradeoff of maintenance burden vs performance. Note also that oversize is a much bigger performance bug than undersize because you pay for it for the entire process, not just startup.
participants (2)
-
Ivan Gerasimov
-
Martin Buchholz