Regression: Slow TypeDescriptor lookups in CachedIntrospectionResults on IBM JVM 6 [SPR-12185]

[spring-projects-issues]

Torsten Hartwig opened SPR-12185 and commented

We upgraded our portal web application from Spring 2.5.6 to 4.0.6 and got into trouble with a very high load, when many users access the frontend.

Every request performs slower with a factor 2 and more with the new Spring version. We tried to reproduce it with Spring 3.2.9, but the load was the same like 2.5.6.

Our application is configured with a parent ear-context to share singletons and every war in that ear has its own web-context. Furthermore every portlet has its own small portlet-app-context. The number of all defined beans is about 1500, 300 are request-scoped.

We investigated that method BeanWrapperImpl.convertForProperty works slow and figured out that behavior of this method has changed from 2.5.6/3.2.9 to 4.X. In 2.5.6/3.2.9 every time BeanWrapperImpl.convertForProperty is called,
a new instance of TypeDescriptor is created. In 4.X every time BeanWrapperImpl.convertForProperty is called, first cachedIntrospectionResults.getTypeDescriptor(pd) is called and if it is null,
cachedIntrospectionResults.addTypeDescriptor is called. And in our case both cache-lookups are much slower than creating a new instance. With a local interim-fix which disabled the cache-lookups in 4.0.6 we could establish
the same performance like 2.5.6/3.2.9 .

Even introducing an optimized cach-lookup

TypeDescriptor addTypeDescriptor(PropertyDescriptor pd, TypeDescriptor td) {
  TypeDescriptor existing = this.typeDescriptorCache.putIfAbsent(pd, td);
  return (existing != null ? existing : td);
}

in CachedIntrospectionResults in Spring 4.1 does not improve the performance.

I don't know the reason for new cache-lookup behavior in CachedIntrospectionResults and the dimension of number of beans that must be declared to become slower with cache-lookups than with creating new instances of TypeDescriptor.

But can you introduce a new spring property that disables the cache lookups and restore "old" behavior? You did something similar with property spring.beaninfo.ignore to skip lookup of BeanInfo classes.

Update (20140912):
Further investgations led to the result that the ConcurrentHashMap-Implementation in our JVM (IBM/WAS 7.0.0.21) may be broken as the get method always returned null, even if the element was inserted just before.
Experimentally replacing ConcurrentHashMap by Collections.synchronizedMap(new HashMap()) seems to resolve this issue. Due to the fact that synchronizedMap uses locking (and can block) we would not use this in our productive environment.

---

Affects: 4.0.7, 4.1 GA

Referenced from: commits b39e66b, 5cd59d0, d1c720c, c52484e

Backported to: 4.0.8

1 votes, 6 watchers

[spring-projects-issues]

Juergen Hoeller commented

I'm switching that type descriptor cache to our own ConcurrentReferenceHashMap in the 4.1 line, in line with similar changes made for 4.1 before. I'm afraid we won't be backporting this to the 4.0.x line though since it leads to a significantly different cache consumption profile. It does come with the benefit of the cache being softly cleared in case of memory demand which is a nice side effect.

This will be available in the upcoming 4.1.1 snapshot. If this does not sufficiently address your performance regression, we'll have to consider introducing a property that controls the use of the cache to begin with, as you suggested.

Juergen

[spring-projects-issues]

Torsten Hartwig commented

Further information:

IBM's implementation of PropertyDescriptor (in beans.jar) is buggy in JVM 1.6; for several PropertyDescriptors pd.quals(pd) returns false. java.util.HashMap works fine in most cases as it does

(localObject = localEntry.key) == paramObject) || ((paramObject != null) && (paramObject.equals(localObject))

while ConcurrentHashMap does not compare both objects but relies on "equals" only.

The problem is probably fixed in JVM 1.7 as PropertyDescriptor.equals(..) (in rt.jar) does a direct comparison

if (this == paramObject) {
return true;
}

which was not present in JVM 1.6.

[spring-projects-issues]

Juergen Hoeller commented

Thanks for the detailed analysis, Torsten... I'll double-check that we're explicitly comparing instance identity for PropertyDescriptors wherever we can.

Juergen

[spring-projects-issues]

Jörg Fraune commented

Update:

Actually the cached objects are org.springframework.beans.GenericTypeAwarePropertyDescriptor; this class does not implement it's own equals-method but relies on java.beans.PropertyDescriptor. The IBM-Implementation does

 public boolean equals(Object paramObject)
  { 
[...]
      int i = ((this.getter == null) && (localPropertyDescriptor.getReadMethod() == null)) || ((this.getter != null) && (this.getter.equals(localPropertyDescriptor.getReadMethod()))) ? 1 : 0;
[...]
  }

with getter being a member variable holding the readMethod. GenericTypeAwarePropertyDescriptor overloads getReadMethod and has it's own private member variable "readMethod". This way the equals method fails if readMethod (or writeMethod, which is handled the same way) is not null.

Probably GenericTypeAwarePropertyDescriptor should define it's own equals-method.

[spring-projects-issues]

Jörg Fraune commented

Probably fix for GenericTypeAwarePropertyDescriptor neccessary

[spring-projects-issues]

Juergen Hoeller commented

Good point, looks like GenericTypeAwarePropertyDescriptor would benefit from its own equals implementation. This would also be easy enough to backport.

That said, with ConcurrentReferenceHashMap now being used in recent 4.1.1 snapshots, we do apply a direct identify comparison as well, in contrast to a regular ConcurrentHashMap. It would be great if you could give that a try... We are going to revise GenericTypeAwarePropertyDescriptor but it'd be great to understand whether the use of ConcurrentReferenceHashMap makes the problem go away on its own.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

The GenericTypeAwarePropertyDescriptor equals/hashCode revision is now also in master (4.1.1) and to be backported to 4.0.8. Note that the use of ConcurrentReferenceHashMap will be constrained to the 4.1.x branch, i.e. just a variant of the GenericTypeAwarePropertyDescriptor revision will be backported to 4.0.x.

Juergen

[spring-projects-issues]

Juergen Hoeller commented

This is available in the latest 4.1.1 and 4.0.8 snapshots now. It would be great if you could give both a try since the implementation unfortunately has to differ in the details due to specific arrangements in each branch.

Juergen

[spring-projects-issues]

Jörg Fraune commented

4.0.8-snapshots are not yet available (newest file at http://maven.springframework.org/snapshot/ is spring-beans-4.0.8.BUILD-20140904.082040-1.jar); tests with 4.1.1 (spring-beans-4.1.1.BUILD-20140917.003734-10.jar) were as fast as our builds with caching disabled (that's great).

Will test 4.0.8-snapshots as they become available, will retest 4.1.1-snapshots as they probably did not yet contain your latest fix (though I think they will not become slow again).

Thanks,

Jörg

[spring-projects-issues]

Juergen Hoeller commented

Oops, the CI build kept failing there because of some rather fragile multi-threading tests. The latest 4.0.x build has passed - a new 4.0.8 snapshot should be available now.

Thanks for the quick turnaround!

Juergen

[spring-projects-issues]

Jörg Fraune commented

Tested 4.0.8-snapshot now, works like a charm (same performance as 4.1.1-snapshot). Will switch to 4.1.1 when it is released.

Thank you,

Jörg

[spring-projects-issues]

Juergen Hoeller commented

That's great to hear! 4.1.1 is due on Sep 30 - in less than two weeks.

Juergen

[spring-projects-issues]

Craig commented

Is this actually a bug in IBM Java's implementation of java.beans.PropertyDescriptor? Has this issue been reported to IBM?

[spring-projects-issues]

Jörg Fraune commented

I don't think so; the implementation of java.beans.PropertyDescriptor (or it's equals-method) looks a little bit clumsy but not wrong. So does the implementation of ConcurrentHashMap in IBMs JDK 1.6, but at least this has been optimized in JDK 1.7.

Jörg

[spring-projects-issues]

Craig commented

FYI, I reported this issue to IBM, service request number 06478,024,649.

There is a difference in behavior between Sun/Oracle/OpenJDK and IBM Java. I have confirmed that Sun/Oracle/OpenJDK 6, 7, and 8 all behave the same way (which I believe is the correct way, as it makes the most sense) and IBM Java 6 behaves what I believe to be the wrong way (I only have access to that version of IBM Java).

The problem is with the .equals() and .hashCode() implementations in java.beans.PropertyDescriptor.
Sun/Oracle/OpenJDK's implementation uses get the getWriteMethod() and getReadMethod() methods, but the IBM implementation uses private variables. The implication is that classes that override java.beans.PropertyDescriptor changing the way read/write methods are retrieved will end up with incorrect equals and hashCode implementations which can be surprising.

This difference is especially problematic as the a lot of Java software is developed on non-IBM JDKs then expected to work on IBM Java and does not (violating the famous "write once, run anywhere" slogan).

Here's a simple test:

import java.beans.IntrospectionException;
import java.beans.PropertyDescriptor;
import java.lang.reflect.Method;


public class TestPropertyDescriptorEquals {
    static class ExtendedPropertyDescriptor extends PropertyDescriptor {
        private final Method readMethod;
        private final Method writeMethod;

        @Override
        public synchronized Method getReadMethod() {
            return readMethod;
        }

        @Override
        public synchronized Method getWriteMethod() {
            return writeMethod;
        }

        public ExtendedPropertyDescriptor(String propertyName,
                Method readMethod, Method writeMethod)
                throws IntrospectionException {
            super(propertyName, null, null);
            this.readMethod = readMethod;
            this.writeMethod = writeMethod;
        }
       
    }

    public static void main(String[] args) throws Exception {
        Method method1 = TestPropertyDescriptorEquals.class.getMethod("getSomething");
        Method method2 = TestPropertyDescriptorEquals.class.getMethod("setSomething", String.class);
       
        ExtendedPropertyDescriptor one = new ExtendedPropertyDescriptor("something", method1, method2);
        ExtendedPropertyDescriptor two = new ExtendedPropertyDescriptor("something", method1, method2);
        if(one.equals(two)){
            System.out.println("GOOD - EQUAL");
        }else{
            throw new IllegalStateException("BAD - NOT EQUAL");
        }
       
        ExtendedPropertyDescriptor three = new ExtendedPropertyDescriptor("something", method1, method1);
        ExtendedPropertyDescriptor four = new ExtendedPropertyDescriptor("something", method2, method2);
        if(three.equals(four)){
            throw new IllegalStateException("BAD - EQUAL");
        }else{
            System.out.println("GOOD - NOT EQUAL");
        }
    }
   
    public String getSomething(){
        return "something";
    }
   
    public void setSomething(String something){
       
    }

}

On Sun/Oracle/OpenJDK, the output is:

GOOD - EQUAL
GOOD - NOT EQUAL

On IBM Java 6, the output is:

Exception in thread "main" java.lang.IllegalStateException: BAD - NOT EQUAL
        at TestPropertyDescriptorEquals.main(TestPropertyDescriptorEquals.java:40)

Here's the exact version of IBM Java I'm using (from "java -version"):

java version "1.6.0"
Java(TM) SE Runtime Environment (build pap6460sr12-20121025_01(SR12))
IBM J9 VM (build 2.4, JRE 1.6.0 IBM J9 2.4 AIX ppc64-64 jvmap6460sr12-20121024_126067 (JIT enabled, AOT enabled)
J9VM - 20121024_126067
JIT  - r9_20120914_26057
GC   - 20120928_AA)
JCL  - 20121014_01