Skip to content

[improve][broker] Optimize and clean up aggregation of topic stats #21361

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.

Sign up for GitHub

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

Jump to bottom
Merged
lhotari merged 1 commit into from
Oct 14, 2023
Merged

[improve][broker] Optimize and clean up aggregation of topic stats #21361

lhotari merged 1 commit into from
Oct 14, 2023

Conversation

@lhotari
Copy link
Member

@lhotari lhotari commented Oct 13, 2023

Motivation

  • getPublishers method gets called twice for every single publisher when aggregating topic stats in the add method of TopicStatsImpl:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/TopicStatsImpl.java

Lines 262 to 263 in 1a352f1

for (int index = 0; index < stats.getPublishers().size(); index++) {
PublisherStats s = stats.getPublishers().get(index);

This is a slight problem since getPublishers method is fairly costly and causes unnecessary garbage:

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/stats/TopicStatsImpl.java

Lines 152 to 158 in 1a352f1

public List<? extends PublisherStats> getPublishers() {
return Stream.concat(publishers.stream().sorted(
Comparator.comparing(PublisherStatsImpl::getProducerName, nullsLast(naturalOrder()))),
publishersMap.values().stream().sorted(
Comparator.comparing(PublisherStatsImpl::getProducerName, nullsLast(naturalOrder()))))
.collect(Collectors.toList());
}

  • The code for aggregating subscription and replication stats is unnecessarily complex.

Modifications

  • extract a variable before the loop to eliminate the problem mentioned above.
  • refactor the code for aggregating subscription and replication stats

Documentation

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

@lhotari lhotari added type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use ready-to-test category/performance Performance issues fix or improvements labels Oct 13, 2023
@lhotari lhotari added this to the 3.2.0 milestone Oct 13, 2023
@lhotari lhotari self-assigned this Oct 13, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 13, 2023
@lhotari
[improve][broker] Optimize and clean up aggregation of topic stats
49d31a8 
- getPublishers is a costly method that is called in each loop twice
- also optimize and clean up code form subscription and replication stats
@lhotari lhotari force-pushed the lh-optimize-topic-publisher-stats branch from 0caea0c to 49d31a8 Compare October 13, 2023 11:33
@heesung-sohn
Copy link
Contributor

heesung-sohn commented Oct 14, 2023

Can we also cache the sorted publisher list? It seems unnecessary to sort the same list repeatedly.

@lhotari lhotari closed this Oct 14, 2023
@lhotari lhotari reopened this Oct 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2023 •
edited
Loading

Codecov Report

Merging #21361 (49d31a8) into master (1a352f1) will increase coverage by 39.83%.
Report is 2 commits behind head on master.
The diff coverage is 80.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #21361       +/-   ##
=============================================
+ Coverage     33.49%   73.32%   +39.83%     
- Complexity    12257    32553    +20296     
=============================================
  Files          1636     1888      +252     
  Lines        127789   140255    +12466     
  Branches      13963    15435     +1472     
=============================================
+ Hits          42798   102846    +60048     
+ Misses        79372    29324    -50048     
- Partials       5619     8085     +2466
Flag Coverage Δ
inttests 24.28% <0.00%> (+0.13%) ⬆️
systests 24.70% <0.00%> (?)
unittests 72.62% <80.00%> (+40.62%) ⬆️

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

Files Coverage Δ
...sar/common/policies/data/stats/TopicStatsImpl.java 94.70% <100.00%> (+41.90%) ⬆️
...licies/data/stats/NonPersistentTopicStatsImpl.java 83.78% <62.50%> (+72.54%) ⬆️

... and 1533 files with indirect coverage changes

@lhotari
Copy link
Member Author

lhotari commented Oct 14, 2023

Can we also cache the sorted publisher list? It seems unnecessary to sort the same list repeatedly.

One possibility is to eliminate the getPublishers method. It looks like it doesn't make much sense at the moment. That could be refactored in another PR.

@lhotari lhotari merged commit d6a56ad into apache:master Oct 14, 2023
vraulji567 pushed a commit to vraulji567/pulsar that referenced this pull request Oct 16, 2023
@lhotari
[improve][broker] Optimize and clean up aggregation of topic stats (a…
9231a81 
…pache#21361)
@TakaHiR07
Copy link
Contributor

TakaHiR07 commented Aug 5, 2025

This issue is occur in branch-3.0, I think it is not just a optimization, but also a bug fix which should cherry-pick this pr to branch-3.0 and branch-3.1 @lhotari

@lhotari
Copy link
Member Author

lhotari commented Aug 5, 2025

This issue is occur in branch-3.0, I think it is not just a optimization, but also a bug fix which should cherry-pick this pr to branch-3.0 and branch-3.1 @lhotari

@TakaHiR07 I think it makes sense to cherry-pick to branch-3.0. We don't maintain branch-3.1 at all.

lhotari added a commit that referenced this pull request Aug 5, 2025
@lhotari
[improve][broker] Optimize and clean up aggregation of topic stats (#…
4fd61ab 
…21361)

(cherry picked from commit d6a56ad)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Aug 6, 2025
@lhotari @nodece
[improve][broker] Optimize and clean up aggregation of topic stats (a…
2fa08d4 
…pache#21361)

(cherry picked from commit d6a56ad)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 20, 2025
@lhotari @ganesh-ctds
[improve][broker] Optimize and clean up aggregation of topic stats (a…
2ddcf97 
…pache#21361)

(cherry picked from commit d6a56ad)
(cherry picked from commit 4fd61ab)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Aug 20, 2025
@lhotari @srinath-ctds
[improve][broker] Optimize and clean up aggregation of topic stats (a…
4ef8a75 
…pache#21361)

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

Reviewers

@rdhabalia rdhabalia rdhabalia approved these changes

@dao-jun dao-jun dao-jun approved these changes

@heesung-sohn heesung-sohn heesung-sohn approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

@tisonkun tisonkun Awaiting requested review from tisonkun

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

@lhotari lhotari

Labels

category/performance Performance issues fix or improvements cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.14 type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use

Projects

None yet

Milestone

3.2.0

Development

Successfully merging this pull request may close these issues.

6 participants

@lhotari @heesung-sohn @codecov-commenter @TakaHiR07 @rdhabalia @dao-jun