Skip to content

Keep subfield memoization keys alive during execution - #285

Open
jadenfix wants to merge 1 commit into
graphql-python:mainfrom
jadenfix:fix/subfield-cache-key-lifetime
Open

jadenfix wants to merge 1 commit into
graphql-python:mainfrom
jadenfix:fix/subfield-cache-key-lifetime

Conversation

@jadenfix

Copy link
Copy Markdown

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 -nEW build 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.

@jadenfix
jadenfix requested a review from Cito as a code owner October 18, 2026 13:07
@codspeed

codspeed Bot commented Oct 19, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 20 untouched benchmarks


Comparing jadenfix:fix/subfield-cache-key-lifetime (97cb5ae) with main (d913a61)

Open in CodSpeed

@Cito Cito left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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_fields

This 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 FieldDetails is no longer a cache hit, just like in GraphQL.js. So the assertion executor.collect_subfields(item_type, list(fields)) is first needs to go.
  • Please don't reuse the same list via fields.clear() and fields.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 and del them.
  • The __del__ tracking is fine, since tuples cannot be weakly referenced. CI also passes on PyPy.
  • Please leave the existing memoizes_collect_subfields_results as 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.

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.

collect_subfields memoization keyed on id() of FieldDetails that are not kept alive returns another field's subfields

2 participants