You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "guan46 (via GitHub)" <gi...@apache.org> on 2025/02/11 16:13:00 UTC

[PR] [improve][ml] Use lock-free queues in InflightReadsLimiter.java to improve performance [pulsar]

guan46 opened a new pull request, #23962:
URL: https://github.com/apache/pulsar/pull/23962

   
   ### Motivation
   
   - Because all changes to queuedHandles are in the lock, there is no need to use thread-safe collections
   
   ### Modifications
   
   - Replace SpscArrayQueue with ArrayDeque.
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: <!-- ENTER URL HERE -->
   
   <!--
   After opening this PR, the build in apache/pulsar will fail and instructions will
   be provided for opening a PR in the PR author's forked repository.
   
   apache/pulsar pull requests should be first tested in your own fork since the 
   apache/pulsar CI based on GitHub Actions has constrained resources and quota.
   GitHub Actions provides separate quota for pull requests that are executed in 
   a forked repository.
   
   The tests will be run in the forked repository until all PR review comments have
   been handled, the tests pass and the PR is approved by a reviewer.
   -->
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "guan46 (via GitHub)" <gi...@apache.org>.
guan46 commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2661302290

   I can't get into pulsar's workspace.Could you help me? @lhotari 
   <img width="695" alt="image" src="https://github.com/user-attachments/assets/b1306450-fb29-4cca-9267-c247a41611d5" />
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "guan46 (via GitHub)" <gi...@apache.org>.
guan46 commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2656892941

   SpscArrayQueue has a memory barrier to ensure data consistency in concurrent situations, whereas ArrayDeque has no memory barrier. ArrayDeque can improve performance in non-concurrent situations @lhotari 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2657046961

   > SpscArrayQueue has a memory barrier to ensure data consistency in concurrent situations, whereas ArrayDeque has no memory barrier. ArrayDeque can improve performance in non-concurrent situations @lhotari
   
   @guan46 Yes, I agree on that. I'm not against the changes in this PR. 
   Please follow-up on my previous comment, https://github.com/apache/pulsar/pull/23962#issuecomment-2651356856 . I don't see a link to the build in your fork.
   
   If you are looking for future contribution opportunities in Pulsar, you can always ask for suggestions on the dev channel in Pulsar Slack. Joining the Pulsar bi-weekly community meeting is a place where contributions are also discussed. I can help to find contribution opportunies with high impact.
   
   Regarding the "performance improvement" aspect: This PR won't improve performance so that you'd be able to measure the difference. One reason for this is that the queue is only used when permits are all used.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "guan46 (via GitHub)" <gi...@apache.org>.
guan46 commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2668971978

   All the tests of my repo have passed, please take a look. @lhotari 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2670922832

   ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23962?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 74.26%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`f933447`)](https://app.codecov.io/gh/apache/pulsar/commit/f93344716de12c97de3ea3ce65eb3325a3e66db9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 926 commits behind head on master.
   
   <details><summary>Additional details and impacted files</summary>
   
   
   [![Impacted file tree graph](https://app.codecov.io/gh/apache/pulsar/pull/23962/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/pulsar/pull/23962?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #23962      +/-   ##
   ============================================
   + Coverage     73.57%   74.26%   +0.69%     
   + Complexity    32624    31929     -695     
   ============================================
     Files          1877     1854      -23     
     Lines        139502   143881    +4379     
     Branches      15299    16352    +1053     
   ============================================
   + Hits         102638   106858    +4220     
   + Misses        28908    28645     -263     
   - Partials       7956     8378     +422     
   ```
   
   | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23962/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/23962/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.75% <16.66%> (+2.17%)` | :arrow_up: |
   | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23962/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `23.18% <16.66%> (-1.14%)` | :arrow_down: |
   | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23962/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.80% <100.00%> (+0.95%)` | :arrow_up: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Files with missing lines](https://app.codecov.io/gh/apache/pulsar/pull/23962?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
   |---|---|---|
   | [...eeper/mledger/impl/cache/InflightReadsLimiter.java](https://app.codecov.io/gh/apache/pulsar/pull/23962?src=pr&el=tree&filepath=managed-ledger%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fbookkeeper%2Fmledger%2Fimpl%2Fcache%2FInflightReadsLimiter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bWFuYWdlZC1sZWRnZXIvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9pbXBsL2NhY2hlL0luZmxpZ2h0UmVhZHNMaW1pdGVyLmphdmE=) | `82.99% <100.00%> (+0.85%)` | :arrow_up: |
   
   ... and [1042 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23962/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   </details>


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #23962:
URL: https://github.com/apache/pulsar/pull/23962


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queues in InflightReadsLimiter.java to improve performance [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2651336622

   @guan46 Just curious to know: how do you know that this improves performance? What's your use case and what are you measuring?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "guan46 (via GitHub)" <gi...@apache.org>.
guan46 commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2661267395

   All the tests of my repo have passed, please take a look.Thank you! @lhotari 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "guan46 (via GitHub)" <gi...@apache.org>.
guan46 commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2670677689

   > …. @guan46 you will be able to join with the link at https://pulsar.apache.org/community/#section-discussions or
   I can access this link and have joined the pulsar community.And the code's been fixed like you said.@lhotari 
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2661349763

   > I can't get into pulsar's workspace.Could you help me? @lhotari 
   > <img width="695" alt="image" src="https://github.com/user-attachments/assets/b1306450-fb29-4cca-9267-c247a41611d5" />
   > 
   
   @guan46 you will be able to join with the link at https://pulsar.apache.org/community/#section-discussions or 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queues in InflightReadsLimiter.java to improve performance [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2651356856

   This PR is correct, but just interested to know more about the motivation behind this PR.
   The original reason to use SpscArrayQueue was due to a plan to remove synchronization from InflightReadsLimiter, but I didn't end up doing that. It should be fine to make the change that's in this PR.
   
   @guan46 Please run CI in your own fork as a first verification step. That ensures that this change doesn't break something and you'll be able to work on the changes without needing someone to spend time in checking if this PR is ok or not.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#issuecomment-2670842837

   /pulsarbot rerun-failure-checks


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] [improve][ml] Use lock-free queue in InflightReadsLimiter since there's no concurrent access [pulsar]

Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23962:
URL: https://github.com/apache/pulsar/pull/23962#discussion_r1961905941


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/cache/InflightReadsLimiter.java:
##########
@@ -82,9 +83,10 @@ public InflightReadsLimiter(long maxReadsInFlightSize, int maxReadsInFlightAcqui
         this.remainingBytes = maxReadsInFlightSize;
         this.acquireTimeoutMillis = acquireTimeoutMillis;
         this.timeOutExecutor = timeOutExecutor;
+        this.maxReadsInFlightAcquireQueueSize = maxReadsInFlightAcquireQueueSize;
         if (maxReadsInFlightSize > 0) {
             enabled = true;
-            this.queuedHandles = new SpscArrayQueue<>(maxReadsInFlightAcquireQueueSize);
+            this.queuedHandles = new ArrayDeque<>(maxReadsInFlightAcquireQueueSize);

Review Comment:
   ArrayDeque takes an initial capacity as the parameter. The purpose is `maxReadsInFlightAcquireQueueSize` is to limit the capacity. It doesn't make sense to allocate the buffer upfront. Simply remove the `maxReadsInFlightAcquireQueueSize` parameter.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org