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

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

Merged
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:

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:

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
- getPublishers is a costly method that is called in each loop twice
- also optimize and clean up code form subscription and replication stats
@heesung-sn
Copy link
Contributor

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

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
82 of 83 checks passed
vraulji567 pushed a commit to vraulji567/pulsar that referenced this pull request Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category/performance Performance issues fix or improvements doc-not-needed Your PR changes do not impact docs ready-to-test type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants