Details
-
Type: Bug
-
Status: Closed
-
Priority: Major
-
Resolution: Won't Fix
-
Affects Version/s: None
-
Fix Version/s: None
-
Component/s: Core
-
Labels:None
Description
Whether or not .clone() exists for a given type, XStream should cache the the result of the lookup.
Performance issues result otherwise (including throwing of NoSuchMethodException).
Activity
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
SerializationMethodInvoker is the same (line 128). It tries to cache, but that is pointless as AbstractReflectionConverter makes a new one in its ctor.