Skip to content

[improve][broker] avoid creating new objects when intercepting - #22790

Merged
dao-jun merged 3 commits into
apache:masterfrom
mattisonchao:fix/small_object
May 28, 2024
Merged

[improve][broker] avoid creating new objects when intercepting#22790
dao-jun merged 3 commits into
apache:masterfrom
mattisonchao:fix/small_object

Conversation

@mattisonchao

Copy link
Copy Markdown
Member

Motivation

When we do performance testing on the cluster. we find an issue which causes creates many small objects.

image

Modifications

  • replace the object with logic.

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

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

@mattisonchao mattisonchao self-assigned this May 28, 2024
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 28, 2024
@mattisonchao mattisonchao added ready-to-test area/broker and removed doc-not-needed Your PR changes do not impact docs labels May 28, 2024
@mattisonchao mattisonchao added this to the 3.4.0 milestone May 28, 2024
@mattisonchao mattisonchao reopened this May 28, 2024
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label May 28, 2024
@mattisonchao mattisonchao changed the title [improve][broker] avoid creating a new object when intercept [improve][broker] avoid creating new objects when intercepting May 28, 2024
@codecov-commenter

codecov-commenter commented May 28, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.24%. Comparing base (bbc6224) to head (22b65d5).
⚠️ Report is 1260 commits behind head on master.

Files with missing lines Patch % Lines
...er/intercept/BrokerInterceptorWithClassLoader.java 87.09% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22790      +/-   ##
============================================
- Coverage     73.57%   73.24%   -0.34%     
- Complexity    32624    32630       +6     
============================================
  Files          1877     1891      +14     
  Lines        139502   141617    +2115     
  Branches      15299    15542     +243     
============================================
+ Hits         102638   103722    +1084     
- Misses        28908    29873     +965     
- Partials       7956     8022      +66     
Flag Coverage Δ
inttests 27.37% <0.00%> (+2.78%) ⬆️
systests 24.78% <0.00%> (+0.46%) ⬆️
unittests 72.25% <87.09%> (-0.60%) ⬇️

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

Files with missing lines Coverage Δ
...er/intercept/BrokerInterceptorWithClassLoader.java 87.50% <87.09%> (-1.57%) ⬇️

... and 404 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.

@dao-jun
dao-jun merged commit 82025b8 into apache:master May 28, 2024
@eolivelli

Copy link
Copy Markdown
Contributor

From the data you pointed out it doesn't see a big deal to have that instance, there are many other cases.
Probably we shold drop ClassloaderSwitcher at all ?
It seems a code smell that in some cases we use it and sometimes not. I am pretty sure that at some point someone will come an revert this change because it sees that this code is different from the other codes that use NarClassLoader

@dao-jun

dao-jun commented May 28, 2024

Copy link
Copy Markdown
Member

Since BrokerInterceptor could execute in the message publish/consume calling chain, I think we should try our best to avoid object allocation.
I support the PR, maybe it smells not good, but, it could work better.

@dao-jun

dao-jun commented May 28, 2024

Copy link
Copy Markdown
Member

Otherwise, all the optimizations for memory/object allocation in the calling chain is meaningless: "It not a big deal to allocate objects/heap memories, let them GC"

@mattisonchao

Copy link
Copy Markdown
Member Author

Since BrokerInterceptor could execute in the message publish/consume calling chai

this is the point here. :)
I observed 100k+ objects here in the tiny cluster.

nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request May 31, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 4, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 4, 2024
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants