Remove the assert in Integer.valueOf() (original) (raw)

Rémi Forax forax at univ-mlv.fr
Tue May 1 11:15:49 UTC 2012

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.MAXVALUE as maximum array size. See: http://bugs.sun.com/bugdatabase/viewbug.do?bugid=7153247 and: java.lang.AbstractCollection.MAXARRAYSIZE

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.MAXVALUE h = Math.min(h, Integer.MAXVALUE - (-low)); /********** little shorter : **********/ // Maximum array size is Integer.MAXVALUE h = Math.min(Integer.MAXVALUE - (-low), Math.max(parseInt(cacheHigh), 127)); } high = h; /********** more simple : **********/ int h = 0; if (cacheHigh != null) { // Maximum array size is Integer.MAXVALUE h = Math.min(Integer.MAXVALUE - (-low), parseInt(cacheHigh)); } high = Math.max(h, 127); /********** no need for variable h : **********/ if (cacheHigh != null) // Maximum array size is Integer.MAXVALUE high = Math.min(Integer.MAXVALUE - (-low), Math.max(parseInt(cacheHigh), 127)); else high = 127; /********** shorter : **********/ high = (cacheHigh == null) ? 127 : // Maximum array size is Integer.MAXVALUE Math.min(Integer.MAXVALUE - (-low), Math.max(parseInt(cacheHigh), 127)); /********** most short : **********/ // Maximum array size is Integer.MAXVALUE high = Math.max(127, Math.min((cacheHigh == null) ? 127 : parseInt(cacheHigh), Integer.MAXVALUE - (-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

More information about the core-libs-dev mailing list