Skip to content

1% possible memory and cpu improvements #3939

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

Open
bbakerman opened this issue Apr 29, 2025 · 1 comment
Open

1% possible memory and cpu improvements #3939

bbakerman opened this issue Apr 29, 2025 · 1 comment
Labels
performance work that is primarily targeted as performance improvements

Comments

@bbakerman
Copy link
Member

bbakerman commented Apr 29, 2025

graphql-java is a mature and optimised library. So its increasingly difficult to find performance improvements that really stand out.

This leads us to the 1% club where we need to do a bunch of work to get a small 1% gain

This issue tries to capture investigations of these 1%'ers and hence be a list of possible micro improvements

Profiler investigations - memory

Running ComplexQueryPeformance as load and using JProfiler

The biggest objects are as follows

Image

"graphql.execution.ExecutionStrategyParameters", 4% of memory
"graphql.execution.ExecutionStrategyParameters$Builder", 4% of memory use
"graphql.execution.ExecutionStepInfo" 2%
"graphql.execution.ExecutionStepInfo$Builder", 2%
"graphql.execution.NonNullableFieldValidator", 2%
"graphql.execution.FetchedValue", 2%

I have PRS that try to reduce all these - eg either remove the builder usage for "transforms" (ExecutionStrategyParameters and ExecutionStepInfo)

or avoid re-allocating per field (NonNullableFieldValidator and FetchedValue)

#3924
#3929
#3934
#3935

The builder ones reduce by memory per type by half right - because we no longer create a builder to actually create a ExecutionStepInfo say

interesting future things to look into

  • "graphql.util.IntraThreadMemoizedSupplier", 4%
    ** why so heavy - do we strictly need it

  • "graphql.schema.FieldCoordinates" 2%
    ** can we avoid this some how - flyweights?

  • "graphql.schema.PropertyFetchingImpl$CacheKey", 2%
    ** can we avoid this some how - flyweights?

  • "com.google.common.collect.SingletonImmutableList", 2%
    ** why so high?? Are we wrapping something for no reason

  • "graphql.schema.DataFetcherFactoryEnvironment", 2%
    ** "graphql.schema.DataFetcherFactoryEnvironment$Builder" 2%
    ** this can go I think

asserts cost cpu and memory

Image

eg :

this.executionStepInfo = assertNotNull(executionStepInfo, () -> "executionStepInfo is null");

These asserts are great for code reasons - but they cost something to run

Can we use the jspecify annotations here and remove these because static analysis is giving us the safety ?

cpu hot spots to look into

  • 1.6% - 1,256 ms - 1,372,469 hot spot inv. graphql.schema.GraphQLCodeRegistry.getDataFetcherImpl

  • 1.5% - 1,190 ms - 7,119,736 hot spot inv. graphql.schema.GraphQLTypeUtil.unwrapNonNull

  • 1.5% - 1,209 ms - 7,088,722 hot spot inv. graphql.execution.ExecutionStepInfo.getUnwrappedNonNullType

  • 1.3% - 1,010 ms - 12,160,902 hot spot inv. graphql.Assert.assertNotNull(java.lang.Object, java.util.function.Supplier)

  • 1.0% - 786 ms - 1,485,657 hot spot inv. graphql.execution.ExecutionStrategy.unboxPossibleDataFetcherResult

This is an interesting one - its called on DataFetch BUT its also called in completeValueForList

meaning can a DF returns a DataFetcherResult that itself is a list of DataFetcherResult of values. Is this even remotely sensible?? Can we kill it in lists the name of 1% ??

@bbakerman bbakerman added the performance work that is primarily targeted as performance improvements label Apr 29, 2025
@bbakerman
Copy link
Member Author

bbakerman commented Apr 29, 2025

Findings

  • "graphql.util.IntraThreadMemoizedSupplier", 4%
    ** why so heavy - do we strictly need it

This is actually saving us work and memory. This wraps the DataFetcherEnvironment creation (and heavy ish object to create) when we end up calling LightDataFetchers - eg PropertyDataFetchers

So while its allocating memory - its saving memory and cpu work.

  • "com.google.common.collect.SingletonImmutableList", 2%
    ** why so high?? Are we wrapping something for no reason

This is from graphql.execution.MergedField#newSingletonMergedField where it runs this code

    private MergedField(Field field, DeferredExecution deferredExecution) {
        this(ImmutableList.of(field), deferredExecution == null ? ImmutableList.of() : ImmutableList.of(deferredExecution));
    }

We have lots of MergedFields and hence we have lots of SingletonImmutableList created

"graphql.schema.FieldCoordinates" 2%
** can we avoid this some how - flyweights?

"graphql.schema.PropertyFetchingImpl$CacheKey", 2%
** can we avoid this some how - flyweights?

We could to flyweights and it saves memory but its slower to run

See #3940 for more details

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance work that is primarily targeted as performance improvements
Projects
None yet
Development

No branches or pull requests

1 participant