Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance improvement with reflection In AJAXRenderer #1195

Open
igieon opened this issue Dec 26, 2023 · 7 comments
Open

Performance improvement with reflection In AJAXRenderer #1195

igieon opened this issue Dec 26, 2023 · 7 comments
Assignees

Comments

@igieon
Copy link

igieon commented Dec 26, 2023

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
Copy link
Collaborator

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

@igieon
Copy link
Author

igieon commented Dec 28, 2023

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
Copy link
Author

igieon commented Dec 28, 2023

Improved version
AJAXRenderer.java

@stephanrauh
Copy link
Collaborator

stephanrauh commented Dec 28, 2023

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
Copy link
Author

igieon commented Dec 28, 2023

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
Copy link
Collaborator

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
Copy link
Author

igieon commented Dec 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants