HashMap sizing was a terrible design mistake, but too much software depends on it, e.g. https://google.github.io/guava/releases/23.0/api/docs/com/google/common/coll... On Tue, Dec 11, 2018 at 1:26 AM Claes Redestad <claes.redestad@oracle.com> wrote:

Hi Peter,

On 2018-12-11 10:02, Peter Levart wrote:

...

Hi Claes,

Haven't checked all changes yet, although it looks like a straightforward swap of Properties for HashMap for intermediary result, but I noticed the following in SystemProps:

265 var cmdProps = new HashMap<String, String>((vmProps.length / 2) + Raw.FIXED_LENGTH);

The HashMap(int) constructor is different from Properties(int) in that for the former, the argument represents the lower bound on the initial size of the table (which is just a rounding of this parameter up to the nearest power of 2). The threshold where the table is resized is calculated as (initialCapacity rounded up to nearest power of 2) * loadFactor. The default load factor is 0.75 which means that the table will be resized in worst case after 3/4 * initialCapacity of elements are inserted into it. In order to guarantee that the table is not resized you have to pass (size * 4 + 2) / 3 to the HashMap constructor, where size is the number of elements added...

I hope I'm not misleading you, I just think this is how HashMap has been from the beginning. Peeking at HashMap code (line 693) it seems that it is doing that:

float ft = (float)newCap * loadFactor; newThr = (newCap < MAXIMUM_CAPACITY && ft < (float)MAXIMUM_CAPACITY ? (int)ft : Integer.MAX_VALUE);

newCap above is holding the initialCapacity constructor parameter rounded up to the nearest power of 2. newThr is the threshold at which the resize occurs.

The Properties(int) constuctor behaves differently as it passes the parameter directly to the underlying ConcurrentHashMap, which says:

* @param initialCapacity The implementation performs internal * sizing to accommodate this many elements.

Fun story, but I noticed this unfortunate difference when I was working on this very patch and brought it up in the team. I think most agrees the CHM behavior is the more intuitive and would have loved to consolidate to that behavior, but the message I've gotten is that it's probably too late to fix: Whichever way you go you get lots of little subtle changes to sizes that may lead to incompabilities in applications/tests that inadvertedly depend on the iteration order or HashMap etc..

That said: Raw typically contains quite a few empty/null values that'll never be put in the map. Enough so that for the applications I've looked at the initialCapacity is already a fair bit higher than it needs to be to avoid resizing. Thus it made little sense to take the maximum possible size and adjust it up even further by factoring in the default load factor.

(Unless you have a *lot* of properties coming in via command line, but I think we should optimize for the common cases...)

Thanks!

/Claes

...

Regards, Peter

On 12/10/18 10:17 PM, Claes Redestad wrote:

...

Hi,

by inverting the order in which the internal property maps are created, we avoid some classloading and get a slightly more efficient code execution profile in System.initPhase1.

Webrev: http://cr.openjdk.java.net/~redestad/8215159/jdk.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8215159

This reduces bytecode executed in initPhase1 from ~48k to ~36k. Not much by any measure, but minimizing System.initPhase1 is important since certain parts of the VM (JIT etc) are blocked from initializing until it's done.

Thanks!

/Claes

Hi Claes, SystemProps.java:   - the import of Collections isn't used. VM.java:  - line 180:  The savedProps doesn't need to be (and was not previously) unmodifiable.    Seems safer this way though since the map at the bottom is not ConcurrentHashMap.    Its not entirely clear who calls getSavedProperties().    Would it be more efficient for savedProps to be the unmodifiable map and not create    one on every call to getSavedProperties. VersionProps.java.template:  - The version properties don't need to be in the savedProps map and could be put only in the Properties.    (save a few cycles and entries). System.java:  - swap lines 805/806 if you decide not to put version props in the map.  - and call VerionProps.init() after the call to createProperties(). Regards, Roger On 12/11/2018 12:41 PM, Mandy Chung wrote:

On 12/10/18 1:17 PM, Claes Redestad wrote:

...

Hi,

by inverting the order in which the internal property maps are created, we avoid some classloading and get a slightly more efficient code execution profile in System.initPhase1.

Webrev: http://cr.openjdk.java.net/~redestad/8215159/jdk.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8215159

This change looks okay to me.

ModuleBootstrap also removes some `jdk.module.*` private system properties during module system initialization but they are only set if module-related command-line options are set.   These properties are not present in the common cases.   These private system properties are the interface between VM and libraries.   There is other mechanism that we can look into to replace using system properties in the future.

Mandy

Hi, On 2018-12-11 19:08, Roger Riggs wrote:

Hi Claes,

SystemProps.java:   - the import of Collections isn't used.

will fix before push.

VM.java:  - line 180:  The savedProps doesn't need to be (and was not previously) unmodifiable.    Seems safer this way though since the map at the bottom is not ConcurrentHashMap.    Its not entirely clear who calls getSavedProperties().    Would it be more efficient for savedProps to be the unmodifiable map and not create    one on every call to getSavedProperties.

VM.getSavedProperties should be removed, but that requires some coordination that is out of scope for this improvement. There's only one caller, and they cache it in a static field. Not wrapping VM.savedProps marginally improves speed of VM.savedProperty which is more used during startup.

VersionProps.java.template:  - The version properties don't need to be in the savedProps map and could be put only in the Properties.    (save a few cycles and entries).

Good point. I have played with code that goes even further (no need to put any of the Raw properties in the savedProps map). We can save maybe another 6k executed bytecode in total, but as it gets unclear what can be found in VM.savedProperties and what can be found in the System properties I decided against it in this patch. Thanks! /Claes

System.java:  - swap lines 805/806 if you decide not to put version props in the map.  - and call VerionProps.init() after the call to createProperties().

Regards, Roger

On 12/11/2018 12:41 PM, Mandy Chung wrote:

...

On 12/10/18 1:17 PM, Claes Redestad wrote:

...

Hi,

by inverting the order in which the internal property maps are created, we avoid some classloading and get a slightly more efficient code execution profile in System.initPhase1.

Webrev: http://cr.openjdk.java.net/~redestad/8215159/jdk.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8215159

This change looks okay to me.

ModuleBootstrap also removes some `jdk.module.*` private system properties during module system initialization but they are only set if module-related command-line options are set.   These properties are not present in the common cases.   These private system properties are the interface between VM and libraries.   There is other mechanism that we can look into to replace using system properties in the future.

Mandy