Re: 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