check_key_fields validation optimization for projects with heavily reused fragments

[apramana]

Summary
Codegen takes substantially longer in v3 compared to v2 in projects with heavily reused fragments due to check_key_fields validation.

Version
3.0.0-beta05

Description
Hello - I am in the process of migrating our project (which makes extensive use of nested and reused fragments) to Apollo 3, and noticed that the codegen task takes significantly longer for us than it does in v2. For comparison, codegen (excluding compilation) on our root module takes ~30s in v2 and ~3 min in v3.

To investigate, I cloned the Apollo compiler and found that most of the time was spent on the checkKeyFields call on fragments. Within check_key_fields.kt, I added a println and noticed that identical paths are being checked more than once. Am I correct in understanding that checkField should only be called once per unique path value (and that selections remain stable)? If so, could a set of checked paths be maintained so that duplicate calls are avoided? This change substantially reduces the time this class takes to run (our root module codegen comes down to 40s with this optimization) but I'm unsure if it breaks the completeness.

[martinbonnin]

Thanks for sending this! We are monitoring the compile time but don't have any tests to detect such regressions at the moment. That's definitely something to add.

Am I correct in understanding that checkField should only be called once per unique path value

In CheckKeyFieldsScope.checkField, parentType might be important too, not sure.

This change substantially reduces the time this class takes to run (our root module codegen comes down to 40s with this optimization)

That's impressive 👏 ! Want to open a pull request and we can go from there?

[BoD]

Hi 👋

Trying to look at this, for now I couldn't reproduce a case where checkKeyFields takes significant time relatively to the rest of the compilation - albeit trying on various test cases, not a real project. Would you mind sharing how you singled out this method in your investigation? If possible it would be awesome if we could also have a look at your schema / operations (we understand if that's not possible - you could also send it privately to benoit.lubek@apollographql.com). Thanks!

[martinbonnin]

@apramana can you check with 3.0.0 and #3720 ? It has an optimization to disable this check early if not needed

[apramana]

@martinbonnin Awesome thanks for the patch, the build speed is much better now. For reference, I tested compilation of Apollo 3.0 GA in a multi-module project using compat codegen (though it probably doesn’t matter which one is used since this bug is in AST generation.)

Also, I’ve posted a PR for optimizing fragment spreads. Please take a look and provide any feedback.

[martinbonnin]

All the thanks goes to @BoD who did all the work there :-)

Thanks for the PR, we'll take a look quickly !

[apramana]

@martinbonnin @BoD I noticed that Apollo React provides the ability to disable normalization for specific types. Does apollo-kotlin support this? (And if so, can this be used to exclude types from validation?)

[martinbonnin]

@apramana we don't have anything like this at the moment. Feel free to open a separate issue to track this.

[martinbonnin]

I'm closing this one in favor of #3966. As of 3.1.0, anyone not using @fieldPolicy or @typePolicy should never bump into this issue.

In the future, we'll want to optimize that part of the codegen but before we optimise, we'll need a measure of what we're optimizing, which is what the above issue is about.