Repository navigation
Conversation
Cito
left a comment
There was a problem hiding this comment.
Thanks a lot, @jadenfix, and thanks @abraira85 for the excellent report in #284.
The analysis is right: this is a regression in 3.3, since 3.2 used the ids of the FieldNodes, which live as long as the document.
I'd like to fix it a bit differently, though, to stay closer to GraphQL.js, which this code is ported from. There, collectSubfields is memoized with memoize3, i.e. on the identity of the fieldDetailsList itself, held in a WeakMap. Using the ids of the list items was a GraphQL-core-only deviation, and it turns out to gain nothing: I counted the cache hits over the whole test suite and both ways of keying get exactly the same hits (27,470 of 29,670 calls). So I'd rather drop the deviation and do what GraphQL.js does. Since lists cannot be weakly referenced, we keep the list in the cache entry instead, which also keeps its id from being reused:
self._relevant_sub_fields: dict[
tuple[GraphQLObjectType, int], tuple[FieldDetailsList, CollectedFields]
] = {} relevant_sub_fields = self._relevant_sub_fields
# Like GraphQL.js, we memoize on the identity of the field_details_list.
# Since lists cannot be weakly referenced, we keep the list alive together
# with the cached value, so that its id cannot be reused during execution.
key = (return_type, id(field_details_list))
cached = relevant_sub_fields.get(key)
if cached is not None:
return cached[1]
collected_fields = collect_subfields(
self.schema,
self.fragments,
self.variable_values,
self.operation,
return_type,
field_details_list,
self.hide_suggestions,
)
relevant_sub_fields[key] = (field_details_list, collected_fields)
return collected_fieldsThis replaces the old comment about the ids of the items. It is also cheaper than the current code: no map(id, ...) for lists with several items, and no tuple copy on a miss.
About the test:
- With this change, a new list with the same
FieldDetailsis no longer a cache hit, just like in GraphQL.js. So the assertionexecutor.collect_subfields(item_type, list(fields)) is firstneeds to go. - Please don't reuse the same list via
fields.clear()andfields.append(). With identity keys, that would be a cache hit, and lists are never mutated after they have been collected anyway. Just use two separate lists anddelthem. - The
__del__tracking is fine, since tuples cannot be weakly referenced. CI also passes on PyPy. - Please leave the existing
memoizes_collect_subfields_resultsas it is, since it is a port of the GraphQL.js test.
Do you want to update the PR accordingly? Otherwise I'm happy to make the changes myself.
The subfield cache keys on
id(FieldDetails)but does not retain those objects. After a field detail is freed, CPython can reuse its identity for another field, returning the earlier field's selection set.Keep an immutable tuple of the key objects alongside each collected result. The existing identity keys and cache hits remain intact, including calls with a different list containing the same objects. Keeping a snapshot also handles callers clearing their input list; references are released with the shared execution cache.
The regression checks key lifetime, cache identity, distinct selection results, and eventual release. The reported reproducer produced 497 incorrect selections in 500 runs on main and none with this change. A separately installed wheel also passed 400 complete GraphQL schema/query/result journeys covering distinct fields, merged selections, and aliases.
Validation: Python 3.14 full suite 3,528 passed / 18 skipped with the required 100% source and test coverage; Python 3.10 floor 3,529 passed / 17 skipped. Ruff check/format, mypy (353 files), and the strict
sphinx -b html -nEWbuild pass. An independent AI reviewer inspected the cache's ownership across sub-executors and reran all 43 executor tests. Prepared with AI assistance.Fixes #284.