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

Reduce memory copy in zstd compression #7681

Merged
merged 4 commits into from
May 31, 2023

Conversation

luyuncheng
Copy link
Contributor

As i see #3577 there is a zstd compression mode.
i think in some scenarios, like init compress buffer, we can save memory copy in ArrayUtil.grow instead of using ArrayUtil.growNoCopy

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented May 23, 2023

@dblock
Copy link
Member

dblock commented May 23, 2023

Curious what the impact on this change is, any benchmarks or ballpark numbers?

cc: @backslasht @mulugetam who were doing some numbers

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Merging #7681 (01bb220) into main (0921ad7) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #7681      +/-   ##
============================================
- Coverage     70.89%   70.67%   -0.22%     
+ Complexity    56242    56130     -112     
============================================
  Files          4682     4682              
  Lines        266102   266100       -2     
  Branches      39070    39070              
============================================
- Hits         188658   188076     -582     
- Misses        61478    62130     +652     
+ Partials      15966    15894      -72     
Impacted Files Coverage Δ
...rg/opensearch/common/settings/ClusterSettings.java 92.50% <ø> (-0.36%) ⬇️
.../index/codec/customcodecs/ZstdCompressionMode.java 84.09% <100.00%> (ø)
.../codec/customcodecs/ZstdNoDictCompressionMode.java 76.71% <100.00%> (ø)

... and 469 files with indirect coverage changes

Signed-off-by: luyuncheng <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu
      1 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=tasks.list/10_basic/tasks_list test}

@reta
Copy link
Collaborator

reta commented May 25, 2023

@dblock looks like a legit fix (of not doing unneeded array copy), wdyt?

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationMultiSearchDuringQueryPhase

@Bukhtawar
Copy link
Collaborator

While this one looks straight forward but its always a good practise to understand the optimisation and check if there are regressions if any with performance

@dblock
Copy link
Member

dblock commented May 30, 2023

@luyuncheng do you have any numbers?

@luyuncheng
Copy link
Contributor Author

luyuncheng commented May 31, 2023

@luyuncheng do you have any numbers?

@dblock i think this optimize just reduce memory copy, mini benchmark can not show the exactly performance. AND it certainly not introduce any performance regression.

i do some mini benchmark with rally: a little promote
Track: http_logs , jvm 1G(less memory trigger more gc), 8core, 16G Mem, codec:zstd

Metric Task Baseline Contender Diff Unit Diff%
Cumulative indexing time of primary shards 260.726 253.455 -7.271 min -2.79%
Cumulative merge time of primary shards 97.1903 95.1604 -2.02988 min -2.09%
Cumulative merge count of primary shards 585 578 -7 -1.20%
Cumulative merge throttle time of primary shards 11.4248 10.4442 -0.98063 min -8.58%
Cumulative refresh time of primary shards 23.4759 23.6735 0.1976 min +0.84%
Cumulative refresh count of primary shards 2085 2047 -38 -1.82%
Cumulative flush time of primary shards 2.22883 2.26052 0.03168 min +1.42%
Cumulative flush count of primary shards 122 121 -1 -0.82%
Total Young Gen GC time 263.871 256.28 -7.591 s -2.88%
Total Young Gen GC count 16495 16437 -58 -0.35%
Total Old Gen GC time 52.601 47.93 -4.671 s -8.88%
Total Old Gen GC count 606 591 -15 -2.48%
Min Throughput index-append 89497.2 94867.8 5370.6 docs/s +6.00%
Mean Throughput index-append 96260.5 97990.2 1729.73 docs/s +1.80%
Median Throughput index-append 96184.2 97639.2 1455.02 docs/s +1.51%
99th percentile latency index-append 1271.33 1250.93 -20.4042 ms -1.60%
99.9th percentile latency index-append 1861 1860.47 -0.52277 ms -0.03%
99.99th percentile latency index-append 2799.13 2337.49 -461.639 ms -16.49%
99th percentile service time index-append 1271.33 1250.93 -20.4042 ms -1.60%
99.9th percentile service time index-append 1861 1860.47 -0.52277 ms -0.03%
99.99th percentile service time index-append 2799.13 2337.49 -461.639 ms -16.49%

@dblock dblock merged commit a0a8a18 into opensearch-project:main May 31, 2023
@dblock dblock added the backport 2.x Backport to 2.x branch label May 31, 2023
@dblock
Copy link
Member

dblock commented May 31, 2023

Looks innocent enough. I merged it. Thanks @luyuncheng!

@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7681-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a0a8a181551c536228fc96bb0f27030b892ae51a
# Push it to GitHub
git push --set-upstream origin backport/backport-7681-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-7681-to-2.x.

@reta
Copy link
Collaborator

reta commented May 31, 2023

@luyuncheng mind please creating a manual backport to 2.x? thank you

reta pushed a commit to reta/OpenSearch that referenced this pull request Jun 1, 2023
* 1. reduce memory copy in zstd compression

Signed-off-by: luyuncheng <[email protected]>

* Add Change log

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
(cherry picked from commit a0a8a18)
@reta
Copy link
Collaborator

reta commented Jun 1, 2023

@luyuncheng mind please creating a manual backport to 2.x? thank you

Backported

@luyuncheng
Copy link
Contributor Author

@luyuncheng mind please creating a manual backport to 2.x? thank you

Backported

Thanks !!!

dblock pushed a commit that referenced this pull request Jun 1, 2023
* 1. reduce memory copy in zstd compression

Signed-off-by: luyuncheng <[email protected]>

* Add Change log

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
(cherry picked from commit a0a8a18)

Co-authored-by: luyuncheng <[email protected]>
austintlee pushed a commit to austintlee/OpenSearch that referenced this pull request Jun 2, 2023
* 1. reduce memory copy in zstd compression

Signed-off-by: luyuncheng <[email protected]>

* Add Change log

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
gaiksaya pushed a commit to gaiksaya/OpenSearch that referenced this pull request Jun 26, 2023
…nsearch-project#7865)

* 1. reduce memory copy in zstd compression

Signed-off-by: luyuncheng <[email protected]>

* Add Change log

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
(cherry picked from commit a0a8a18)

Co-authored-by: luyuncheng <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
* 1. reduce memory copy in zstd compression

Signed-off-by: luyuncheng <[email protected]>

* Add Change log

Signed-off-by: luyuncheng <[email protected]>

---------

Signed-off-by: luyuncheng <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants