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

fix: always copy once after top get #577

Merged
merged 13 commits into from Jan 24, 2024
Merged

fix: always copy once after top get #577

merged 13 commits into from Jan 24, 2024

Conversation

AsterDY
Copy link
Collaborator

@AsterDY AsterDY commented Jan 18, 2024

Background

sonic.Get() does copy the entired JSON and refer it to returned partial json. This behavior is neither CPU-friendly nor memory-safe: Copy entire JSON is time consuming, and returned JSON consumes more memory especially if it is cached or hold for a long term. Thus, We decide to not copy input JSON bytes, but copy returned partial JSON instead.

What's the effect

After this change, most normal services may gain a little CPU improve on sonic.Get(), and cache-like services will be less possible to be OOM.
Only problem is for services who exists data-race on input JSON bytes, the panic may be more frequent. However, we believe this is a rare case in practice, and we DONOT expect any data race on input JSON.

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (63f4c07) 78.57% compared to head (ae7e496) 74.26%.

Files Patch % Lines
ast/search.go 70.83% 4 Missing and 3 partials ⚠️
api.go 60.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
- Coverage   78.57%   74.26%   -4.32%     
==========================================
  Files          69       63       -6     
  Lines       10823     8435    -2388     
==========================================
- Hits         8504     6264    -2240     
+ Misses       1942     1830     -112     
+ Partials      377      341      -36     

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

@AsterDY AsterDY enabled auto-merge (squash) January 18, 2024 13:13
@AsterDY AsterDY merged commit 9f2242e into main Jan 24, 2024
30 checks passed
@AsterDY AsterDY deleted the fix/get_oom branch January 24, 2024 12:29
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.

None yet

3 participants