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

feat: make dedupe faster at I/O level #3237

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alpha-ulrich
Copy link

Summary:
Uses Arc to ensure that dedupe requests do not generate new clones of the response, but rather point to the same response using the Atomically referenced counter.

Issue Reference(s):
Fixes #3028

/claim #3028

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests. [Modified relevant tests after code changes]
  • I have updated the documentation accordingly. [No update required for documentation]
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Jan 3, 2025
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.

Project coverage is 86.32%. Comparing base (e3a6026) to head (d3850c3).

Files with missing lines Patch % Lines
src/core/data_loader/dedupe.rs 85.18% 4 Missing ⚠️
src/core/ir/eval.rs 75.00% 1 Missing ⚠️
src/core/ir/eval_io.rs 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3237      +/-   ##
==========================================
- Coverage   86.32%   86.32%   -0.01%     
==========================================
  Files         282      282              
  Lines       28798    28821      +23     
==========================================
+ Hits        24860    24879      +19     
- Misses       3938     3942       +4     

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

@tusharmath tusharmath added the ci: lint Automatically fix the linters issues and make a commit label Jan 4, 2025
@tusharmath
Copy link
Contributor

@alpha-ulrich Thanks!
Can you add some e2e benchmarks to see if there is an observed difference in performance? We eventually end up cloning the data in the PR, so I am curious to see if Arc is making a difference in performance.

@alpha-ulrich
Copy link
Author

@tusharmath sure, I'll see what can be done, later today.
I'll have a look at the graphql-benchmarks repository, to see if I can reuse some parts from there to accomplish this.

If there are any specific metrics that you would want reported, do let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🙋 Bounty claim ci: lint Automatically fix the linters issues and make a commit type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: make dedupe faster at I/O level.
2 participants