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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable key fields check if unnecessary and optimize its perf #3720

Merged
merged 5 commits into from
Dec 15, 2021

Conversation

BoD
Copy link
Contributor

@BoD BoD commented Dec 15, 2021

This branch investigates a performance issue in the compiler around check key fields, that has been reported.

  • Do not check key fields if there are no typePolicy directives
  • Cache implementedTypes and keyFields
  • Cache collectFields entirely

馃憜 when profiling, these changes seem to show the check runs faster, however we still don't have a convincing repro steps where it was taking a significant time overall, so caution must be taken

/**
* Returns whether the `typePolicy` directive is present on at least one object in the schema
*/
fun hasAnyTypePolicyDirectives(): Boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: rename to hasTypeWithTypePolicy ?


private val keyFieldsCache = mutableMapOf<String, Set<String>>()
fun keyFields(name: String) = keyFieldsCache.getOrPut(name) {
schema.keyFields(name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤

parentType: String,
implementedTypes: Set<String>,
) = collectFieldsCache.getOrPut("$selections $parentType $implementedTypes") {
collectFields(selections, parentType, implementedTypes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any idea the cache hit ratio for this? I'd expect it to be relatively low?

Copy link
Contributor Author

@BoD BoD Dec 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the test in the PR I can see that the method was run 2693 times overall, and only 405 times after making it cached. So 2288 duplicated calls, meaning ~80% cache hit?

checkKeyFields(it, options.schema, allFragmentDefinitions)
// Check if all the key fields are present in operations and fragments
// (do this only if there are key fields as it may be costly)
if (options.schema.hasAnyTypePolicyDirectives()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤

import kotlin.time.measureTime

@OptIn(ExperimentalTime::class, ApolloExperimental::class)
class PerfTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do everything from CodegenTest ? CodegenTest writes a measurements file containing the codegen time IIRC so it might be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I really don't think we should push that test and indeed it should be one of the regular tests ran by CodegenTest (in fact it already does because where the files are located, and fails because I didn't generate the .expected 馃槄). But since it's fairly slow, and we may want to continue investigations, it's easier to have a dedicated test temporarily. If we want to merge this branch, let's remove or move it first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to merge the other fixes quickly. My moving the test to another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 let's do it!

@BoD BoD changed the title Check key fields perf investigation Disable key fields check if unnecessary and optimize its perf Dec 15, 2021
@BoD BoD marked this pull request as ready for review December 15, 2021 10:13
* Returns whether the `typePolicy` directive is present on at least one object in the schema
*/
fun hasTypeWithTypePolicy(): Boolean {
return typeDefinitions.values.filterIsInstance<GQLObjectTypeDefinition>().any { objectType ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for our future selves: we'll want to support @typePolicy on interfaces and unions (see #3356). This will need to be updated when this happens

Copy link
Contributor

@martinbonnin martinbonnin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 馃憤

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.

None yet

2 participants