[improve][ml] Reduce object allocation when iterate on LongPairRangeSet by lifepuzzlefun · Pull Request #18357 · apache/pulsar

lifepuzzlefun · 2022-11-06T15:41:25Z

Fixes #18356

Motivation

support direct perform logic on (long,long) pair without Range and PostionImpl object creation

Modifications

add LongPairRangeSet.forEachWithRangeBoundMapper
change ManagedCursorImpl.buildIndividualDeletedMessageRanges and LongPairRangeSet.size to use this function
add unit test to check forEach and forEachWithRangeBoundMapper return the same result.

Verifying this change

Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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

If the box was checked, please highlight the changes

Documentation

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

…angeSet` (apache#18356)

lifepuzzlefun · 2022-11-07T17:11:20Z

after jmh test. we can see both mem allocation and throughtput improves with this pr

lifepuzzlefun · 2022-11-07T17:12:08Z

origin jmh test here
lifepuzzlefun@ffca95d

the result json can achieve here
https://gist.github.com/WJL3333/bff704b2783b617fc0bc93413113e197

just upload to https://jmh.morethan.io/ get the picture.

lifepuzzlefun · 2022-11-07T17:17:43Z

@merlimat @codelipenghui @Technoboy- @poorbarcode PTAL

coderzc · 2022-11-08T05:19:11Z

    }

+    @Override
+    public <O> void forEachWithRangeBoundMapper(LongPairConsumer<O> rawRangeBoundMapper,


There is a lot of duplication of code with forEach, can it be optimized?

thanks for review. the generic for range require for <T extands Comparable<T>> which is used in Range<T> but for iteration ops we may transform to non Comparable object. i try to reuse but we need the MLDataFormats.MessageRange implements Comparable

Ichange the signature and optimize the code. please take a look~

lifepuzzlefun · 2022-11-11T02:50:37Z

@codelipenghui @eolivelli @Jason918 @lhotari @HQebupt PTAL

codelipenghui · 2022-11-11T10:57:55Z

-                return rangeList.size() <= config.getMaxUnackedRangesToPersist();
-            });
+
+            individualDeletedMessages.forEachWithRangeBoundMapper(


Can we just have a callback like

individualDeletedMessages.forEach((l1, l2, u1, u2) -> { })

Nice suggestion ! I have done coding. please take a look again.

codelipenghui · 2022-11-16T13:12:58Z

/pulsarbot run-failure-checks

codecov-commenter · 2022-11-19T06:15:09Z

Codecov Report

Merging #18357 (653a7e9) into master (a2c1534) will decrease coverage by 2.05%.
The diff coverage is 36.84%.

@@             Coverage Diff              @@
##             master   #18357      +/-   ##
============================================
- Coverage     47.23%   45.17%   -2.06%     
- Complexity    10430    10660     +230     
============================================
  Files           692      758      +66     
  Lines         67766    72750    +4984     
  Branches       7260     7805     +545     
============================================
+ Hits          32009    32865     +856     
- Misses        32192    36200    +4008     
- Partials       3565     3685     +120

Flag	Coverage Δ
unittests	`45.17% <36.84%> (-2.06%)`	⬇️

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

Impacted Files	Coverage Δ
...pache/bookkeeper/mledger/impl/RangeSetWrapper.java	`43.39% <33.33%> (-2.61%)`	⬇️
...che/bookkeeper/mledger/impl/ManagedCursorImpl.java	`37.06% <38.46%> (+0.24%)`	⬆️
...tent/NonPersistentDispatcherMultipleConsumers.java	`40.74% <0.00%> (-12.35%)`	⬇️
...apache/pulsar/broker/service/TopicListService.java	`42.62% <0.00%> (-12.30%)`	⬇️
.../org/apache/bookkeeper/mledger/impl/EntryImpl.java	`69.62% <0.00%> (-11.27%)`	⬇️
...e/pulsar/broker/service/EntryBatchIndexesAcks.java	`82.14% <0.00%> (-10.72%)`	⬇️
...keeper/mledger/impl/cache/RangeEntryCacheImpl.java	`45.79% <0.00%> (-6.31%)`	⬇️
.../pulsar/broker/service/PublishRateLimiterImpl.java	`58.00% <0.00%> (-6.00%)`	⬇️
...ava/org/apache/pulsar/broker/service/Consumer.java	`66.19% <0.00%> (-5.98%)`	⬇️
...oker/service/schema/SchemaRegistryServiceImpl.java	`59.45% <0.00%> (-5.41%)`	⬇️
... and 129 more

lifepuzzlefun · 2022-11-20T03:20:54Z

All CI passed in my local repository. see lifepuzzlefun#2

315157973 · 2022-11-20T03:37:37Z

+        @Override
+        public void forEachRawRange(RawRangeProcessor action) {
+            for (Range<T> range : asRanges()) {
+                LongPair lowerEndpoint = this.rangeEndPointConsumer.apply(range.lowerEndpoint());


When a range does not set the lower boundary, whether there will be NPE here？

thanks for review, after check the code, all range is insert by LongPairRangeSet.addOpenClosed(long,long,long,long) i think there is no chance to get a Range which boundary is not set. maybe i missed somewhere, correct me if im wrong

Technoboy-

LGTM

tisonkun · 2022-12-06T10:41:43Z

cc @codelipenghui @Technoboy- @Jason918 shall we merge this patch now?

…et (apache#18357)

The forEachRawRange callback used to append each range and then return rangeList.size() <= maxRanges, which kept iteration alive until the list reached maxRanges + 1 entries. One extra range was therefore serialized on every persist, and a cursor whose exact ack-hole count equaled maxRanges + 1 raised a spurious overflow warning even though nothing was actually truncated. Switch to the check-before-add pattern (symmetric with the existing buildBatchEntryDeletionIndexInfoList logic) and derive the overflow signal from a MutableBoolean set when the callback refuses an extra range. MutableBoolean is used rather than AtomicBoolean because this method runs under the cursor write lock and does not need atomic semantics; the existing MutableBoolean usage in readEntries (around line 2219) follows the same convention. The regression lived since apache#3819 (2019), when the original stream().limit(N) was translated to a forEach callback without keeping the strict cap. PR apache#18357 later migrated to forEachRawRange and carried the bug forward. Extend testPersistOverflowRangesCounter to reopen the cursor after ledger.close() in the overflow scenario and assert that exactly maxRanges entries are recovered, guarding against a reintroduction. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

[improve][broker] Reduce object allocation when iterate on `LongPairR…

505779f

…angeSet` (apache#18356)

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Nov 6, 2022

lifepuzzlefun added 3 commits November 6, 2022 23:57

fix npe

775c7fe

add unit test

fc18417

add unit test

40a4d53

lifepuzzlefun changed the title ~~[improve][broker] Reduce object allocation when iterate on LongPairRangeSet~~ [improve][ml] Reduce object allocation when iterate on LongPairRangeSet Nov 8, 2022

coderzc reviewed Nov 8, 2022

View reviewed changes

codelipenghui reviewed Nov 11, 2022

View reviewed changes

reuse builder

94bb275

lifepuzzlefun force-pushed the avoid_object_alloction_when_iterator_long_pair_range_set branch from d257efa to 94bb275 Compare November 12, 2022 14:51

fix checkstyle and javadoc

265dec6

lifepuzzlefun force-pushed the avoid_object_alloction_when_iterator_long_pair_range_set branch from 06935e6 to 265dec6 Compare November 12, 2022 17:23

codelipenghui approved these changes Nov 16, 2022

View reviewed changes

codelipenghui assigned lifepuzzlefun Nov 16, 2022

codelipenghui added this to the 2.12.0 milestone Nov 16, 2022

codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/ML labels Nov 16, 2022

codelipenghui requested review from 315157973, Technoboy-, coderzc, congbobo184, eolivelli, gaoran10 and mattisonchao and removed request for coderzc November 16, 2022 13:13

codelipenghui added the ready-to-test label Nov 16, 2022

Technoboy- closed this Nov 19, 2022

Technoboy- reopened this Nov 19, 2022

lifepuzzlefun added 2 commits November 19, 2022 12:25

fix checkstyle

2429b91

fix wrong input

bb21602

fix wrong input param

653a7e9

315157973 reviewed Nov 20, 2022

View reviewed changes

Technoboy- approved these changes Nov 23, 2022

View reviewed changes

coderzc approved these changes Nov 23, 2022

View reviewed changes

Jason918 approved these changes Nov 24, 2022

View reviewed changes

Technoboy- merged commit 12b864e into apache:master Dec 7, 2022

Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022

[improve][ml] Reduce object allocation when iterate on LongPairRangeS…

9e45832

…et (apache#18357)

Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022

[improve][ml] Reduce object allocation when iterate on LongPairRangeS…

50bf034

…et (apache#18357)

lifepuzzlefun added a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023

[improve][ml] Reduce object allocation when iterate on LongPairRangeS…

9a612b6

…et (apache#18357)

ng-galien mentioned this pull request Apr 17, 2026

[improve][ml] Warn and emit metric when cursor ack state exceeds persist limits #25548

Merged

1 task

Uh oh!

Conversation

lifepuzzlefun commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Uh oh!

lifepuzzlefun commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifepuzzlefun commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lifepuzzlefun commented Nov 7, 2022

Uh oh!

coderzc Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

lifepuzzlefun Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

lifepuzzlefun Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

lifepuzzlefun commented Nov 11, 2022

Uh oh!

codelipenghui Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

lifepuzzlefun Nov 12, 2022

Choose a reason for hiding this comment

Uh oh!

codelipenghui commented Nov 16, 2022

Uh oh!

codecov-commenter commented Nov 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lifepuzzlefun commented Nov 20, 2022

Uh oh!

315157973 Nov 20, 2022

Choose a reason for hiding this comment

Uh oh!

lifepuzzlefun Nov 20, 2022

Choose a reason for hiding this comment

Uh oh!

Technoboy- left a comment

Choose a reason for hiding this comment

Uh oh!

tisonkun commented Dec 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lifepuzzlefun commented Nov 6, 2022 •

edited

Loading

lifepuzzlefun commented Nov 7, 2022 •

edited

Loading

lifepuzzlefun commented Nov 7, 2022 •

edited

Loading

codecov-commenter commented Nov 19, 2022 •

edited

Loading