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: Avoid memory copy in RESTful search and query #37674

Merged
merged 5 commits into from
Nov 19, 2024

Conversation

bigsheeper
Copy link
Contributor

Custom jsonRender that encodes JSON data directly into the response stream, it uses less memory since it does not buffer the entire JSON structure before sending it, unlike c.JSON in HTTPReturn, which serializes the JSON fully in memory before writing it to the response.
Benchmark testing shows that, using the custom render incurs no performance loss and reduces memory consumption by nearly 50%.

issue: #37671

@sre-ci-robot sre-ci-robot added the size/L Denotes a PR that changes 100-499 lines. label Nov 14, 2024
@mergify mergify bot added dco-passed DCO check passed. kind/bug Issues or changes related a bug labels Nov 14, 2024
@bigsheeper
Copy link
Contributor Author

Benchmark:
10MB

cpu: Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
BenchmarkHTTPReturn
BenchmarkHTTPReturn/test_HTTPReturn
BenchmarkHTTPReturn/test_HTTPReturn-12         	      87	  13127452 ns/op	27992718 B/op	      34 allocs/op
BenchmarkHTTPReturn/test_HTTPReturnStream
BenchmarkHTTPReturn/test_HTTPReturnStream-12   	      87	  12804875 ns/op	14361636 B/op	      31 allocs/op
PASS

1KB

cpu: Intel(R) Core(TM) i7-8700 CPU @ 3.20GHz
BenchmarkHTTPReturn
BenchmarkHTTPReturn/test_HTTPReturn
BenchmarkHTTPReturn/test_HTTPReturn-12         	  231253	      5026 ns/op	    8639 B/op	      28 allocs/op
BenchmarkHTTPReturn/test_HTTPReturnStream
BenchmarkHTTPReturn/test_HTTPReturnStream-12   	  230566	      4769 ns/op	    7582 B/op	      28 allocs/op
PASS

Copy link
Contributor

mergify bot commented Nov 14, 2024

@bigsheeper cpp-unit-test check failed, comment rerun cpp-unit-test can trigger the job again.

@bigsheeper
Copy link
Contributor Author

rerun cpp-unit-test

Copy link

codecov bot commented Nov 14, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 3 lines in your changes missing coverage. Please review.

Project coverage is 82.46%. Comparing base (b6612e0) to head (6ef5d64).
Report is 3 commits behind head on master.

Current head 6ef5d64 differs from pull request most recent head 17de623

Please upload reports for the commit 17de623 to get more accurate results.

Files with missing lines Patch % Lines
internal/distributed/proxy/httpserver/utils.go 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #37674      +/-   ##
==========================================
- Coverage   82.90%   82.46%   -0.44%     
==========================================
  Files        1067     1068       +1     
  Lines      164517   165259     +742     
==========================================
- Hits       136391   136279     -112     
- Misses      22668    23543     +875     
+ Partials     5458     5437      -21     
Components Coverage Δ
Client 61.25% <ø> (-10.91%) ⬇️
Core ∅ <ø> (∅)
Go 83.16% <86.36%> (-0.04%) ⬇️
Files with missing lines Coverage Δ
internal/distributed/proxy/httpserver/handler.go 97.89% <ø> (ø)
...nternal/distributed/proxy/httpserver/handler_v1.go 91.00% <100.00%> (ø)
...nternal/distributed/proxy/httpserver/handler_v2.go 91.69% <100.00%> (ø)
...ternal/distributed/proxy/httpserver/json_render.go 100.00% <100.00%> (ø)
...nternal/distributed/proxy/httpserver/request_v2.go 97.33% <ø> (ø)
...distributed/proxy/httpserver/timeout_middleware.go 79.09% <ø> (ø)
...ernal/distributed/proxy/httpserver/wrap_request.go 95.63% <ø> (ø)
internal/distributed/proxy/httpserver/wrapper.go 100.00% <ø> (ø)
internal/distributed/proxy/listener_manager.go 77.27% <ø> (ø)
internal/distributed/proxy/httpserver/utils.go 84.22% <50.00%> (-0.15%) ⬇️

... and 82 files with indirect coverage changes

---- 🚨 Try these New Features:

Copy link
Contributor

mergify bot commented Nov 15, 2024

@bigsheeper E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@czs007
Copy link
Collaborator

czs007 commented Nov 16, 2024

/approve
/lgtm

@xiaofan-luan
Copy link
Collaborator

/lgtm
/approve

@sre-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bigsheeper, czs007, xiaofan-luan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@bigsheeper
Copy link
Contributor Author

/run-cpu-e2e

Copy link
Contributor

mergify bot commented Nov 18, 2024

@bigsheeper E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@czs007
Copy link
Collaborator

czs007 commented Nov 19, 2024

/run-cpu-e2e

@czs007 czs007 added this to the 2.5.0 milestone Nov 19, 2024
@bigsheeper bigsheeper added the PR | need cherry-pick need cherry pick to other branches label Nov 19, 2024
Copy link
Contributor

mergify bot commented Nov 19, 2024

@bigsheeper E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@sre-ci-robot sre-ci-robot removed the lgtm label Nov 19, 2024
Copy link
Contributor

mergify bot commented Nov 19, 2024

@bigsheeper cpp-unit-test check failed, comment rerun cpp-unit-test can trigger the job again.

Copy link
Contributor

mergify bot commented Nov 19, 2024

@bigsheeper E2e jenkins job failed, comment /run-cpu-e2e can trigger the job again.

@czs007
Copy link
Collaborator

czs007 commented Nov 19, 2024

/lgtm

@czs007 czs007 added ci-passed manual-pass manually set pass before ci-passed labeled labels Nov 19, 2024
@sre-ci-robot sre-ci-robot merged commit b3fc530 into milvus-io:master Nov 19, 2024
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved ci-passed dco-passed DCO check passed. kind/bug Issues or changes related a bug lgtm manual-pass manually set pass before ci-passed labeled PR | need cherry-pick need cherry pick to other branches size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants