BootsFaces-OSP icon indicating copy to clipboard operation
BootsFaces-OSP copied to clipboard

Performance improvement with reflection In AJAXRenderer

Open igieon opened this issue 1 year ago • 7 comments

In bootfaces 1.5 is not caching reflection invocation for ajax get methods this is significant slow down when you have really many awesome icons without ajax. I override implemnentation with caching with help of class ConcurrentReferenceHashMap. I just put this class into class directory so that has precedence with original and rendering speed up come up with 20% improvement of speed (like from 100ms to 80ms). AJAXRenderer.java

igieon avatar Dec 26 '23 16:12 igieon

Sounds like a simple change with almost no negative impact. I suggest we'll add it.

stephanrauh avatar Dec 28 '23 19:12 stephanrauh

Only problem i use ConcurrentReferenceHashMap from spring, but i see that spring is licensed under apache v2 license same as this project so we can copy it and remove spring imports only keep notes. I probably can make slightly modification of code because ConcurrentReferenceHashMap I can do it as pull request on which branch i need start pull request ?

igieon avatar Dec 28 '23 20:12 igieon

Improved version AJAXRenderer.java

igieon avatar Dec 28 '23 21:12 igieon

If you extract the content of the for loop into a dedicated method, it's probably even faster because the optimizing compiler of the JVM kicks in earlier.

However, I'm confused by the equalsIgnoreCase bit. I've left the Java world a long time ago, so I'm not sure, but ignoring the case doesn't feel right to me. That's not the Java way. Or is it?

if (m.getParameterCount() == 0
      && m.getReturnType() == String.class
      && nameOfMethod.equalsIgnoreCase(m.getName())) {
            methodsCache.put(cacheName, m);
            return m;
}

stephanrauh avatar Dec 28 '23 22:12 stephanrauh

nameOfMethod.equalsIgnoreCase(m.getName())

I take from https://github.com/TheCoder4eu/BootsFaces-OSP/blob/master/src/main/java/net/bootsfaces/component/ajax/AJAXRenderer.java#L215 For extracting for loop i don't see reason because later on it still be inlined. I don't change internal logic just add cache for finding reflection method.

igieon avatar Dec 28 '23 22:12 igieon

Just give it a try. Maybe it improves performance, maybe it doesn't. The compiler optimizes short methods earlier than long methods. Inlining is only one of many optimizations.

stephanrauh avatar Dec 28 '23 22:12 stephanrauh

nameOfMethod.equalsIgnoreCase i think was added because events are generated from jquery something as i look into code and and its not consistent naming for this in this project. So just not be consistent its used this one not java style but more like javascript style. And from unwinding i can see only way to put it out for compiler optimization just is just using this method

    private static boolean acceptMethod(Method m, String nameOfMethod) {
        return m.getParameterCount() == 0
            && m.getReturnType() == String.class
            && nameOfMethod.equalsIgnoreCase(m.getName());
        
    }
    
    private static Method findMethod(Class<?> clazz, String nameOfMethod) {
        String cacheName = clazz.getCanonicalName() + '.' + nameOfMethod;
        Method methodCached = methodsCache.get(cacheName);
        if (methodCached == null && !methodsCache.containsKey(cacheName)) {
            for (Method m : clazz.getMethods()) {
                if (acceptMethod(m, nameOfMethod)) {
                    methodsCache.put(cacheName, m);
                    return m;
                }
            }
        }
        return methodCached;
    }

but just adding one more call to stack i don't see benefit and if i put cache outside to the methond than i dont like it form redability point.

igieon avatar Dec 29 '23 00:12 igieon