[Codegen]: Partial fragments

[igor-florescu-ck]

We are currently migrating to the new Apollo codegen.

The new codegen tool runs for over 30 minutes and uses a lot of memory to generate the latest Swift types, and our CI is failing to complete. I suspect this thread might be related to apollographql/apollo-ios#3434.

I am opening this pull request to propose changes and gather feedback from the repo authors.

The current flow:
Whenever the RootFieldBuilder builds named fragments via buildNamedFragmentSpread. The codegen flow will eventually reuse/create a previously built fragment from BuiltFragmentStorage. Once resolved, a NamedFragment type would be returned by the codegen flow and will merge the Entity fields to build up the EntitySelectionTree in the Selection Set which references the named fragment.

The fragments resolved from BuiltFragmentStorage are eventually referenced to it's previously created swift file.

Some fragments can contain very large amounts of Entity fields which might not be relevant for referencing intents.

With this pull request, I suggest adding support for partial NamedFragment to reduce the amount of data merged into the containing EntitySelectionTree.

The intention of partial fragments is to return a NamedFragment which contain entities necessary for referencing intents. (ex: field for inlineFragments, references for referencedFragments etc.)

In this example, I am naively reducing the number of entities that have FieldPaths deeper than 3 (?)
I was also considering an experimental flag along to the config, however I've limited the amount of changes to gather initial feedback.

Why 3? Assumption: entities containing the fields to be spread into the containing inline fragment + underlying fragments and their fields. (maybe rather than naively filtering a depth of 3 a better approach would be to filter by definition/GraphQLType)

I'm not however familiar with all edge cases and would appreciate any suggestions and reviews.

With this change, the same output is generated against our schema, but the duration is reduced from 30 minutes to 3 minutes, and memory usage has decreased to 400 MB.

[netlify]

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	2c49f9c

[netlify]

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	2c49f9c

[apollo-cla]

@igor-florescu-ck: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[AnthonyMDev]

I'm looking at this now, but I'm having a hard time wrapping my head around how this is even passing tests right now. I would assume that cutting off the depth at 3 would make some merged fields be missing. I also haven't worked in this code in a while, and it's quite complex, so I need to regain some context. I'll keep looking into it.

Can you expand on what this means:

In theory, the generation flow needs the types and entities to spread, fragments, and their underlying entities, hence the hardcoded 'count < 4'.

I feel like I'm missing something about why this is adequate. My hunch is that we just don't have any unit tests that actually exceed a depth of 3.

[AnthonyMDev]

I've added a new unit test in PR #570 that merges entities from a fragment with a depth of 4. If you rebase this PR on main now, that test will fail. This is the simplest case in which the partial fragments with a depth of 3 do not work.

While I agree that for almost all cases, having a depth of 3 (or 4 for that matter) will create models that serve their purpose. The issue is that because they are incomplete, selection set initializers will be incorrect. Missing the property accessors on the merged models, while not completely and exhaustively correct, is not a huge issue. Users could access the fragment models to access those fields.

But we use the calculated merged selections to generate the initializers for the models. This means those initializers will be missing fields. If you initialize the models with missing fields and then access those fields by accessing the named fragments that do contain them, you will get a crash, additionally, trying to write the incomplete models to the cache will cause a crash.

Any shortcuts like this that actually make the models not exhaustively correct is going to have that problem. It's actually why the experimental feature for turning off fragment field merging is incompatible with selection set initializers. If you try to enable both features, you get an error.

---

The fact that you report getting the same output with this partial fragment workaround implemented makes me assume that you are already using the config option to disable fragment field merging and thus, are not using selection set initializers. Is that assumption correct?

Currently, disabling field merging only shortcuts the traversal of the merged selection set trees when calculating the merged selections. I wonder if we could add some logic to prevent the mergeAllSelectionsIntoEntitySelectionTrees from being called in the first place if named fragment field merging is disabled. That may be able to give you the performance improvements you're looking for. I'm not sure if that would cause other edge cases to arise.

[AnthonyMDev]

I've implemented the idea that I had in my last comment, and I believe it works!!! Please check out this PR: #571 and tell me if it runs for you.

This only works when fragment field merging is disabled, but as far as I can tell it's working. I'm concerned there might be some edge cases, but I can't come up with any reason why there would be, so I'm hoping this solves it!

[igor-florescu-ck]

Hi @AnthonyMDev,

Thanks for getting back on this. I indeed had initializers disabled, but field merging was enabled. This workaround carried the Entities for those fragments to be merged.

I first started looking at mergeAllSelectionsIntoEntitySelectionTrees; however, this partial approach seemed easier for me to move forward at the time.

I pulled your changed from fragment-field-merging-disabled-performance and it's significantly faster and memory usage manageble.

I will try run it against our project and let you know.

[igor-florescu-ck]

I'm looking at this now, but I'm having a hard time wrapping my head around how this is even passing tests right now. I would assume that cutting off the depth at 3 would make some merged fields be missing. I also haven't worked in this code in a while, and it's quite complex, so I need to regain some context. I'll keep looking into it.

Can you expand on what this means:

In theory, the generation flow needs the types and entities to spread, fragments, and their underlying entities, hence the hardcoded 'count < 4'.

I feel like I'm missing something about why this is adequate. My hunch is that we just don't have any unit tests that actually exceed a depth of 3.

@AnthonyMDev Apologies for the confusion. This hardcoded value was not meant to be taken seriously (it was for review purposes, even though it works just fine with our queries).

I was hoping to get your teams input on this PR so that I can find a way to filter out unnecessary entities.

The idea here was to iterate through the Field Path and leave the necessary Entities to populate:

1. Field merging
2. Initializers
3. Type references
4. Nested fragments

And remove anything else.
As opposed to: #571 my thought was to keep field merging enabled
Disabling Field Merging might not be too important for us.
Thanks

[AnthonyMDev]

Ah, I see. I'm not sure what "unnecessary entities" you expect to filter out though? If you have fragment field merging off, then you would filter out the same things that PR #571 prevents from being merged into the trees in the first place. But if you have field merging enabled, I don't think there are any selections you can filter out and still have safe and fully correct models.

Either way, if you are filtering anything out at all, initializers are going to be a no-go. Selection set initializers require that all fields are calculated and merged to ensure that the initializer has all of the parameters needed to successfully create the model. If you filter a single selection out, you now have initializers that will crash on access or throw errors when written to the normalized cache.

[igor-florescu-ck]

Hi @AnthonyMDev,

Thanks for getting back on this. I indeed had initializers disabled, but field merging was enabled. This workaround carried the Entities for those fragments to be merged.

I first started looking at mergeAllSelectionsIntoEntitySelectionTrees; however, this partial approach seemed easier for me to move forward at the time.

I pulled your changed from fragment-field-merging-disabled-performance and it's significantly faster and memory usage manageble.

I will try run it against our project and let you know.

Hi @AnthonyMDev,
I've tried #571 and looks good so far with our project with field merging disabled.

Thanks for the follow up.
It prevents merging entities and speeds up the tool considerably.
Closing this PR in favour of: #571