Skip to content

don't purge the results from buffer #752

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

Merged
merged 1 commit into from
Aug 1, 2025

Conversation

januszmk
Copy link
Contributor

I tried upgrading graphqlite lib in my project and I was hit by this issue: #729

I am making pr that removes purging from the buffer, but if there is different way to fix the issue, please let me know

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.81%. Comparing base (53f9d49) to head (c3f7884).
⚠️ Report is 118 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #752      +/-   ##
============================================
- Coverage     95.72%   94.81%   -0.91%     
- Complexity     1773     1838      +65     
============================================
  Files           154      175      +21     
  Lines          4586     4998     +412     
============================================
+ Hits           4390     4739     +349     
- Misses          196      259      +63     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@oojacoboo
Copy link
Collaborator

@oprypkhantc was there a reason to purge results from the prefetch buffer? I'm inclined to merge this, but would like to hear your reasoning for adding.

@autaut03
Copy link

autaut03 commented Aug 1, 2025

@oojacoboo Wasn't me who added that. git blame reports #702 as the source. I guess the purging is done because the result for that entity should already exist, so logically there's no reason to store anything other than the result itself.

But I don't know how it's actually implemented, so don't quote me on that. @sudevva you remember any details?

@sudevva
Copy link
Contributor

sudevva commented Aug 1, 2025

I guess the main reason is that the buffer will never be cleared in other case. I need to check why I added this

@autaut03
Copy link

autaut03 commented Aug 1, 2025

We should get a new context (along with the whole prefetch buffer) on each new request, so it will get cleared, just a bit later. It should result in higher memory usage before the response is generated, right?

@sudevva
Copy link
Contributor

sudevva commented Aug 1, 2025

purgeResult can be safely removed, most likely
And if we're targeting php>=8 WeakMap can be used as a replacement for SplObjectStorage

@oojacoboo oojacoboo merged commit d87de70 into thecodingmachine:master Aug 1, 2025
17 of 20 checks passed
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.

5 participants