TheCoder4eu / BootsFaces-OSP

BootsFaces - Open Source Project
Apache License 2.0
247 stars 102 forks source link

Performance improvement with reflection In AJAXRenderer #1195

Open igieon opened 8 months ago

igieon commented 8 months ago

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

stephanrauh commented 8 months ago

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

igieon commented 8 months ago

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 commented 8 months ago

Improved version AJAXRenderer.java

stephanrauh commented 8 months ago

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;
}
igieon commented 8 months ago
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.

stephanrauh commented 8 months ago

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.

igieon commented 8 months ago

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.