Remove the assert in Integer.valueOf()

[Rémi Forax]

On 05/01/2012 12:09 AM, Ulf Zibis wrote:
> Hi Rémi,
>
> Am 28.04.2012 00:42, schrieb Rémi Forax:
>>> While you are there:
>>> IntegerCache.cache/high/low are static final, so should be named 
>>> _upper case_.
>>
>> Not done because the system property is also spelled in lower case.
> Hm, but does that justify violating very common code conventions?
> IMO it's inconvenient while reading the code.

Yes and no. I agree that it's again the code convention
but I think that it better outline the relation with the system property.
Given that all IDEs can be tweaked to display static finals with a 
different color/stroke,
I think this code should not be changed.

>
>>
>>>
>>> Another optimization:
>>>     public static Integer valueOf(int i) {
>>>         if (i >= 0) {
>>>             if (i <= IntegerCache.HIGH)
>>>                 return IntegerCache.POS[i];
>>>         } else {
>>>             if (i >= IntegerCache.LOW)
>>>                 return IntegerCache.NEG[~i];
>>>         }
>>>         return new Integer(i);
>>>     }
>>>
>>
>> Can you explain a little bit more why it's more efficient ?
> 1. Compare against 0 is little faster than against a constant which 
> must be loaded as operand.

Ok, less bytecode but no impact when JITed

> 2. As positive arguments should be more frequent, my version prevents 
> from calculating the index every time:
>     IntegerCache.cache[i + (-IntegerCache.low)]

I believe the JIT can emit a mov in that case, I will check.

> 3. Client VM would profit from less bytecode instructions to go 
> through, as there is less inlining.

Again, I need to test.

>
>> Given that low and high are constant for the JIT, I'm not sure the 
>> current code
>> do bounds checking at all.
> I do not understand this. 'i' is not constant, so should be checked.

I was thinking about the array bound checking.

>
> ==========================================
>
> We must be careful with Integer.MAX_VALUE as maximum array size.
> See: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7153247
> and: java.lang.AbstractCollection.MAX_ARRAY_SIZE

This one is another bug, but you will have trouble with 
Integer.MAX_VALUE - (-low).

Rémi

>
> Additionally, I think, *the assert is superfluous at all*, as 'high' 
> can never be smaller than 127.
> You can see this, if you compact the code:
>
>         static {
>             // high value may be configured by property
>             String cacheHigh =
>                 
> sun.misc.VM.getSavedProperty("java.lang.Integer.IntegerCache.high");
>             /********** no need for variable i : **********/
>             int h = 127;
>             if (cacheHigh != null) {
>                 h = Math.max(parseInt(cacheHigh), 127);
>                 // Maximum array size is Integer.MAX_VALUE
>                 h = Math.min(h, Integer.MAX_VALUE - (-low));
>                 /********** little shorter : **********/
>                 // Maximum array size is Integer.MAX_VALUE
>                 h = Math.min(Integer.MAX_VALUE - (-low),
>                         Math.max(parseInt(cacheHigh), 127));
>             }
>             high = h;
>             /********** more simple : **********/
>             int h = 0;
>             if (cacheHigh != null) {
>                 // Maximum array size is Integer.MAX_VALUE
>                 h = Math.min(Integer.MAX_VALUE - (-low),
>                         parseInt(cacheHigh));
>             }
>             high = Math.max(h, 127);
>             /********** no need for variable h : **********/
>             if (cacheHigh != null)
>                 // Maximum array size is Integer.MAX_VALUE
>                 high = Math.min(Integer.MAX_VALUE - (-low),
>                         Math.max(parseInt(cacheHigh), 127));
>             else
>                 high = 127;
>             /********** shorter : **********/
>             high = (cacheHigh == null) ? 127 :
>                     // Maximum array size is Integer.MAX_VALUE
>                     Math.min(Integer.MAX_VALUE - (-low),
>                             Math.max(parseInt(cacheHigh), 127));
>             /********** most short : **********/
>             // Maximum array size is Integer.MAX_VALUE
>             high = Math.max(127, Math.min((cacheHigh == null) ? 127 :
>                     parseInt(cacheHigh), Integer.MAX_VALUE - (-low)));
>
>             assert IntegerCache.high >= 127;
>
>             // range [-128, 127] must be interned (JLS7 5.1.7)
>             cache = new Integer[(high - low) + 1];
>             for(int j = low, k = 0; k < cache.length; k++, j++)
>                 cache[k] = new Integer(j);
>         }
>
>
> -Ulf
>