Skip to content

[improve][broker] Add ref count for sticky hash to optimize the performance of Key_Shared subscription#19167

Merged
codelipenghui merged 4 commits intoapache:masterfrom
codelipenghui:penghui/performance-redelivery-controller
Jan 11, 2023
Merged

[improve][broker] Add ref count for sticky hash to optimize the performance of Key_Shared subscription#19167
codelipenghui merged 4 commits intoapache:masterfrom
codelipenghui:penghui/performance-redelivery-controller

Conversation

@codelipenghui
Copy link
Copy Markdown
Contributor

@codelipenghui codelipenghui commented Jan 10, 2023

Motivation

Add ref count for sticky hash to optimize the performance of the Key_Shared subscription.

flamegraph-long

The root cause is here. We are going to go through all the redelivery messages to check if contains the hash from a given sticky hash set.

Modifications

Introduce a ref count map for the sticky hash set to make the method containsStickyKeyHashes works more efficiently.

With this fix

image

Verifying this change

Updated the existing test to cover the new changes.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@codelipenghui codelipenghui self-assigned this Jan 10, 2023
@codelipenghui codelipenghui added ready-to-test type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Jan 10, 2023
@codelipenghui codelipenghui added this to the 2.12.0 milestone Jan 10, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Jan 10, 2023
Copy link
Copy Markdown
Member

@mattisonchao mattisonchao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, It will be better to make it to be thread-safe? Because we use two maps to store the state, In the thread race condition, maybe it will introduce a non-consensus state.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 10, 2023

Codecov Report

❌ Patch coverage is 77.14286% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.22%. Comparing base (b05fddb) to head (5098773).
⚠️ Report is 3026 commits behind head on master.

Files with missing lines Patch % Lines
...ervice/persistent/MessageRedeliveryController.java 77.14% 3 Missing and 5 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19167      +/-   ##
============================================
+ Coverage     45.64%   47.22%   +1.58%     
+ Complexity    11043    10703     -340     
============================================
  Files           773      713      -60     
  Lines         74463    69716    -4747     
  Branches       8018     7492     -526     
============================================
- Hits          33986    32926    -1060     
+ Misses        36687    33074    -3613     
+ Partials       3790     3716      -74     
Flag Coverage Δ
unittests 47.22% <77.14%> (+1.58%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ervice/persistent/MessageRedeliveryController.java 70.00% <77.14%> (+6.58%) ⬆️

... and 112 files with indirect coverage changes

🚀 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.

Copy link
Copy Markdown
Contributor

@poorbarcode poorbarcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I wonder if there has a better data structure to work with than ConcurrentLongLongHashMap if the operation on hashesRefCount is thread-safe.

@codelipenghui codelipenghui merged commit 7431381 into apache:master Jan 11, 2023
@codelipenghui codelipenghui deleted the penghui/performance-redelivery-controller branch January 11, 2023 03:01
codelipenghui added a commit that referenced this pull request Jan 11, 2023
…rmance of Key_Shared subscription (#19167)

(cherry picked from commit 7431381)
codelipenghui added a commit that referenced this pull request Jan 11, 2023
…rmance of Key_Shared subscription (#19167)

(cherry picked from commit 7431381)
codelipenghui added a commit that referenced this pull request Jan 11, 2023
…rmance of Key_Shared subscription (#19167)

(cherry picked from commit 7431381)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Feb 28, 2023
…rmance of Key_Shared subscription (apache#19167)

(cherry picked from commit 7431381)
(cherry picked from commit 0ac194b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.9.5 release/2.10.4 release/2.11.1 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants