Cloneables.clone(..) does not cache clone() methods between invocations.

SerializationMethodInvoker is the same (line 128). It tries to cache, but that is pointless as AbstractReflectionConverter makes a new one in its ctor.

SerializationMethodInvoker has a cache member (as mentioned), however it is not caching method instances, it is somewhat separate to that that optimization

SerializationMethodInvoker: I don't understand your comment. It is caching the Method instances. Additionally it is fine that each ARC implementation has its own SMI, because each registered converter will handle a distinct set of types. There's no need to share the cache between converter instances and therefore we do not need a static using weak references for keys and values. The caches exists per XStream instance like all others.

Cloneable: I don't see a performance problem in the current context of XStream either. Cloning is nothing XStream does on a frequent base. Currently it is performing one cloning attempt for every marshalling process. That does hardly qualify as performance problem.

Run a single test, and tell me whether your opinion is unchanged:

BooleanFieldsTest.testBinaryValues()
Add breakpoints to the 'Method result = type.getDeclaredMethod(name, parameterTypes);' ..... line (127?) and the catch{} block that follows, as well as the constructor to XStream

Note that for the single test invocation, for a single shared XStream instance, method Musican.readResolve() is sought four times, and the NoSuchMethodException is also thrown four times consequentially.

I believe that it is correct to cache the fact that the method is missing, and that XStream's current implementation is not caching in the way that I think is correct.

The test I indicate is just one of many within XStream's test-base that exhibits the same behavior.

A client of mine of doing lots of profiling presently, and confused as to why 'new Method' caused by XStream is 10% of of the allocations per request (dispensing with those from app startup).

Cloneables's use of getMethod...

Repeat the test above for GregorianCalendarConverterTest.testCalendar()

Same results: needless second getMethod, and second throw/catch of NoSuchMethodException when caching would have done the right thing.

SimpleMethodInvoker's caching of method lookups is broken. Here's the fix

Replace entire method 'getMethod' like so:

 
    private Method getMethod(Class type, String name, Class[] parameterTypes,
                             boolean includeBaseclasses) {
        final String typeName = type.getName();
        StringBuffer sb = new StringBuffer(typeName.length() + name.length() + 7);
        String key = sb.append(typeName).append('.').append(name).append('.')
                .append(includeBaseclasses).toString();
        Object resultOb = cache.get(key);
        if (resultOb != null) {
            return (resultOb == NO_METHOD) ? null : ((Method) resultOb);
        }
        try {
            Method result = type.getDeclaredMethod(name, parameterTypes);
            result.setAccessible(true);
            cache.put(key, result);
            return result;
        } catch (NoSuchMethodException e) {
            cache.put(key, NO_METHOD);
            type = type.getSuperclass();
            if (includeBaseclasses) {
                if (type != null) {
                    // recurse into super class, and cache that result too
                    return getMethod(type, name, parameterTypes, true);
                } else {
                    return null; // Object.class has no parent.
                }
            } else {
                return null; // don't recurse into super class.
            }
        }
    }

Sorry, dude, but your version currupts the cache. It took me some hours to verify it, but see yourself with the new test SerializationNestedWriteObjectsTest.testCachingInheritedWriteObject. However, this is not hypothetical, the same scenario applies when serializing a synchronized ArrayList.

Won't fix is regarding the Cloneable. As said on the dev list, the much better approach is to use a NameCoder that does not have to be cloned and that can handle names to encode statically.

SerializationMethodInvoker: I've implemented a new (and much smarter) version now for the cache and I can see a drop of the calls for getMethod there by ~25% (running XStream's acceptance tests).

Problem fixed with SerializationMethodInvoker - thanks