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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 1268: Don't cache unserializable objects #1280

Open
wants to merge 2 commits into
base: 8.x-4.x
Choose a base branch
from

Conversation

darvanen
Copy link
Contributor

@darvanen darvanen commented Jun 8, 2022

No description provided.

@darvanen
Copy link
Contributor Author

darvanen commented Jun 8, 2022

See #1268

@Kingdutch
Copy link
Contributor

Hey @darvanen ,

Thanks for the pull request. To help maintainers decide on whether this PR is the right fix for the issue could you add the following:

  1. A description of the change you're making (from reading code I believe the cache is being changed to cache the un-parsed schema rather than the parsed schema?)

  2. Ideally a benchmark to compare the two scenario's. We're changing what is being cached and thus also what operation is now being done more often. The assumption in the original caching implementation was that parsing the schema is slow and that unserializing a large PHP object is faster. Instead the only thing we're now caching is possibly (depending on Schema implementation) an assembly of a few files from disk. While the parsing would still be done on every request.

These two additions help in evaluating this against alternative solutions. For example talking to the GraphQL library maintainers to see if there is perhaps a more cache-friendly representation for a parsed schema.

@darvanen
Copy link
Contributor Author

darvanen commented Jun 9, 2022

Thanks @Kingdutch,
I opened a request with webonyx/graphql as well and it seems to have been received favourably:
webonyx/graphql-php#1164

The discussion around the problem I'm seeking to solve including a benchmark is on issue 1268 which I linked to in a previous comment
#1268
Although the benchmark doesn't cover this exact change.

But it would be much better if this class was made serializable so I suggest we postpone this issue to see how the external one goes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants