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

getParameterCount() can be used instead of getParameterTypes().length and getParameters().length #10333

Closed
clara0 opened this issue Feb 20, 2022 · 2 comments · Fixed by #10334
Assignees
Labels
area/core kind/enhancement Categorizes a PR related to an enhancement status/ready Ready to be merged
Milestone

Comments

@clara0
Copy link
Contributor

clara0 commented Feb 20, 2022

Description

Replace occurrences of getParameterTypes().length and getParameters().length with getParameterCount().

Targets
    Occurrences of 'getParameterTypes().length' in Project
Found Occurrences in Project  (9 usages found)
    Production  (8 usages found)
        Unclassified  (8 usages found)
            keycloak-common  (2 usages found)
                org.keycloak.common.util.reflections  (2 usages found)
                    Types.java  (2 usages found)
                        isCompatible(Method, Method)  (2 usages found)
                            166 if (method.getParameterCount() != intfMethod.getParameterTypes().length) return false;
                            168 for (int i = 0; i < method.getParameterTypes().length; i++)
            keycloak-ldap-federation  (1 usage found)
                org.keycloak.storage.ldap  (1 usage found)
                    LDAPUtils.java  (1 usage found)
                        getUserModelProperties()  (1 usage found)
                            326 && m.getParameterTypes().length > 0) {
            keycloak-oidc-client-adapter-pom  (1 usage found)
                adapters/oidc/as7-eap6/as7-adapter-spi/src/main/java/org/keycloak/adapters/jbossweb  (1 usage found)
                    JBossWebPrincipalFactory.java  (1 usage found)
                        findJBossGenericPrincipalConstructor()  (1 usage found)
                            166 if (c.getParameterTypes().length == 9 &&
            keycloak-server-spi-private  (4 usages found)
                org.keycloak.models.utils.reflection  (4 usages found)
                    MethodPropertyImpl.java  (4 usages found)
                        MethodPropertyImpl(Method)  (2 usages found)
                            52 } else if (method.getParameterTypes().length > 0) {
                            62 } else if (method.getParameterTypes().length != 1) {
                        getSetterMethod(Class<?>, String)  (1 usage found)
                            154 if (methodName.startsWith(SETTER_METHOD_PREFIX) && method.getParameterTypes().length == 1) {
                        getGetterMethod(Class<?>, String)  (1 usage found)
                            166 if (method.getParameterTypes().length == 0) {
    Test  (1 usage found)
        Unclassified  (1 usage found)
            integration-arquillian-tests-base  (1 usage found)
                org.keycloak.testsuite.migration.cluster  (1 usage found)
                    MultiVersionClusterTest.java  (1 usage found)
                        createCacheAndGetFromServer(ContainerInfo)  (1 usage found)
                            279 .anyMatch(c -> c.getParameterTypes().length == 0 )) {

Targets
    Occurrences of 'getParameters().length' in Project
Found Occurrences in Project  (1 usage found)
    Test  (1 usage found)
        Unclassified  (1 usage found)
            integration-arquillian-tests-base  (1 usage found)
                org.keycloak.testsuite.admin  (1 usage found)
                    PermissionsTest.java  (1 usage found)
                        assertGettersEmpty(Object, String...)  (1 usage found)
                            1976 if (m.getParameters().length == 0 && m.getName().startsWith("get") && !ignoreList.contains(m.getName())) {

Discussion

No response

Motivation

getParameterTypes().length and getParameters().length are inefficient because they create an array clone, while getParameterCount() directly returns the length.

Details

No response

@ahus1
Copy link
Contributor

ahus1 commented Mar 10, 2022

Thanks for reporting this, I see you already prepared a PR for this.

I wonder: did you find this via a performance test, or did it have in impact for you? I'd like to understand more about the background/motivation for this PR. Thanks!

@clara0
Copy link
Contributor Author

clara0 commented Mar 10, 2022

@ahus1 I noticed this problem initially in wildfly when taking a look at the code, then saw that other repositories also had the same issue. While I don't think this issue is particularly significant enough to show up on performance tests, the array clones put a strain on the GC.

@ahus1 ahus1 added status/ready Ready to be merged and removed status/triage labels Mar 17, 2022
clara0 added a commit to clara0/keycloak that referenced this issue Mar 17, 2022
hmlnarik pushed a commit that referenced this issue Mar 18, 2022
@stianst stianst added this to the 18.0.0 milestone Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core kind/enhancement Categorizes a PR related to an enhancement status/ready Ready to be merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants