Skip to content

avoid allocating in hasDirective#4026

Closed
xuorig wants to merge 1 commit intographql-java:masterfrom
xuorig:has-directive-perf-fix
Closed

avoid allocating in hasDirective#4026
xuorig wants to merge 1 commit intographql-java:masterfrom
xuorig:has-directive-perf-fix

Conversation

@xuorig
Copy link

@xuorig xuorig commented Jun 27, 2025

hasDirective on DirectivesContainer sounds cheap but actually allocates/copies/does a groupingBy on every call. There is no need to group by in this case, and we can cheaply iterate over getDirectives instead, which does not copy/allocate anything.

@bbakerman
Copy link
Member

Ahh I can see why we are in efficient - in the interfaces we tried to save "code" by making a default method do the work

But interfaces cant have state so this should have been done once on construction but rather now its done per call

    /**
     * This will return a Map of the all directives that are associated with a {@link graphql.language.Node}, including both repeatable and non repeatable directives.
     *
     * @return a map of all directives by directive name
     */
    default Map<String, List<Directive>> getDirectivesByName() {
        return ImmutableMap.copyOf(allDirectivesByName(getDirectives()));
    }

    /**
     * Returns all of the directives with the provided name, including repeatable and non repeatable directives.
     *
     * @param directiveName the name of the directives to retrieve
     *
     * @return the directives or empty list if there is not one with that name
     */
    default List<Directive> getDirectives(String directiveName) {
        return getDirectivesByName().getOrDefault(directiveName, emptyList());
    }

@bbakerman bbakerman self-requested a review June 27, 2025 22:48
@bbakerman bbakerman added this to the 25.x breaking changes milestone Jun 27, 2025
@bbakerman
Copy link
Member

bbakerman commented Jun 28, 2025

Again while I think this mostly fixes the issue I thought I would do a more systemic PR that made sure all the DirectivesContainer methods are more memory efficient.

#4027

I think this is better since the other methods beyond hasDirective will be more efficient and well

@xuorig
Copy link
Author

xuorig commented Jun 28, 2025

Closing in favor of #4027, thanks @bbakerman !

@xuorig xuorig closed this Jun 28, 2025
@dondonz dondonz removed this from the 25.x breaking changes milestone Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants