Optimize memory usage: support to shrink for pendingAcks map #14515

lordcheng10 · 2022-03-01T13:59:18Z

Modifications

In order to optimize memory consumption, in the consumer class, the map used by the pendingAcks variable needs to support automatic shrink.

The original pendingAcks variable is defined by the ConcurrentLongLongPairHashMap class of bookkeeper. In order to be independent of the bookkeeper release version, this class is extracted into pulsar as a common class, similar to the ConcurrentLongHashMap class.

See the PR corresponding to bookkeeper: apache/bookkeeper#3061.

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

doc-required

(If you need help on updating docs, create a doc issue)
no-need-doc

(Please explain why)
doc

(If this PR contains doc changes)

eolivelli · 2022-03-01T14:01:15Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

+            category = CATEGORY_SERVER,
+            doc = "Whether ConcurrentLongLongPairHashMap supports automatic shrinking," +
+                    "the default is false, which means automatic shrinking is not supported.")
+    private boolean autoShrink = false;


we shouldn't add this knob.
and if we really want the name should be more narrow and point exactly to the thing that we are changing

remove this configuration? Or modify the configuration name? like: autoShrinkForConcurrentLongLongPairHashMap?

it is only for pendingAcks at the moment, so the name should reflect it.
it is not for every ConcurrentLongLongPairHashMap

like this?
autoShrinkForPendingAcks

Fixed. PTAL,thanks! @eolivelli

rename autoShrink to autoShrinkForConsumerPendingAcksMap;

eolivelli · 2022-03-01T14:02:05Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Topic.java

+     * @param autoShrink whether ConcurrentLongLongPairHashMap supports automatic shrinking.
+     * @return
+     */
+    CompletableFuture<Consumer> subscribe(SubscriptionOption option, boolean autoShrink);


I believe that adding a new method for "autoShrink" is not necessary.
also the variable name should be more meaningful.

Do you mean adding the autoShrink parameter directly on the original method?

this is global parameter, maybe you have another way to pass this way.
every Consumer will use the same value.

if you add a "parameter" in the function it means that it would be possible to see different values and this is not true

I believe that overall we don't need a configuration flag at all.

your change is good and you demonstrated that it is needed

cc @merlimat

this is global parameter, maybe you have another way to pass this way.
every Consumer will use the same value.
if you add a "parameter" in the function it means that it would be possible to see different values and this is not true

OK , I will fix

I believe that overall we don't need a configuration flag at all.

Do you think the autoShrink configuration item should not be added? When defining pendingAcks, set it directly to true? For example:
this.pendingAcks = ConcurrentLongLongPairHashMap.newBuilder()
.autoShrink(true)
.expectedItems(256)
.concurrencyLevel(1)
.build();

Fixed. PTAL,thanks! @eolivelli
In the ServerCnx class, define a static variable autoShrinkForConsumerPendingAcksMap instead of passing parameters;

lordcheng10 · 2022-03-02T05:51:38Z

/pulsarbot run-failure-checks

Jason918 · 2022-03-02T07:28:32Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

@@ -704,6 +704,12 @@
    )
    private boolean isAllowAutoUpdateSchemaEnabled = true;

+    @FieldContext(
+            category = CATEGORY_SERVER,
+            doc = "Whether ConcurrentLongLongPairHashMap supports automatic shrinking,"


supports -> enable ?

And let's add some description about when it's better to enable this auto shinking.

Jason918 · 2022-03-02T07:31:30Z

...n/src/main/java/org/apache/pulsar/common/util/collections/ConcurrentLongLongPairHashMap.java

+ *
+ * <p>Keys <strong>MUST</strong> be &gt;= 0.
+ */
+public class ConcurrentLongLongPairHashMap {


I think it's better to copy unit test too.

Fixed
@Jason918 PTAL,thanks!

lordcheng10 · 2022-03-02T12:52:09Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-03-02T12:56:00Z

/pulsarbot run-failure-checks

lordcheng10 · 2022-03-02T13:02:44Z

/pulsarbot run-failure-checks

eolivelli

we are closer.
but we cannot use a 'static' variable

eolivelli · 2022-03-02T13:11:53Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

@@ -205,6 +205,7 @@
    private boolean autoReadDisabledPublishBufferLimiting = false;
    private final long maxPendingBytesPerThread;
    private final long resumeThresholdPendingBytesPerThread;
+    public static boolean autoShrinkForConsumerPendingAcksMap;


this shouldn't be static

In addition to defining static variables, or passing parameters through methods, I have not thought of other ways. @eolivelli Do you have any other good ideas?

Suggested change

public static boolean autoShrinkForConsumerPendingAcksMap;

@Getter

private boolean autoShrinkForConsumerPendingAcksMap;

I think this works.

I think this works.

The question here is: how do we pass autoShrinkForConsumerPendingAcksMap to the Consumer class, if we don't define autoShrinkForConsumerPendingAcksMap as static and pass the method parameters.

Aha, I found a solution, the configuration can be accessed in this way in the consumer class:

subscription.getTopic().getBrokerService()
.getPulsar().getConfiguration().isAutoShrinkForConsumerPendingAcksMap()

Fixed.
@eolivelli PTAL,thanks

The question here is: how do we pass autoShrinkForConsumerPendingAcksMap to the Consumer class, if we don't define autoShrinkForConsumerPendingAcksMap as static and pass the method parameters.

What I mean is like the handling of preciseDispatcherFlowControl.

eolivelli

+1

great work !

lordcheng10 · 2022-03-04T01:54:14Z

@Jason918 PTAL,thanks!

lordcheng10 · 2022-03-04T02:15:05Z

@merlimat PTAL,thanks!

lordcheng10 · 2022-03-05T08:08:06Z

ping

…14515)

(cherry picked from commit e747b8f)

…14515) (cherry picked from commit e747b8f) (cherry picked from commit 2cbf9f4)

…14515) (cherry picked from commit e747b8f) (cherry picked from commit aeb0929)

…14515) (e747b8f)

support to shrink for pendingAcks map

46da1bb

github-actions bot assigned lordcheng10 Mar 1, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 1, 2022

lordcheng10 changed the title ~~support to shrink for pendingAcks map~~ Optimize memory usage: support to shrink for pendingAcks map Mar 1, 2022

eolivelli requested changes Mar 1, 2022

View reviewed changes

lordcheng10 added 7 commits March 1, 2022 23:05

1. rename autoShrink to autoShrinkForConsumerPendingAcksMap;

3bf9b2e

check style

3a86de1

use AtomicIntegerFieldUpdater for size

2508c75

check style

9269688

check style

53cef2e

check style

898a5cb

check style

307209e

Jason918 reviewed Mar 2, 2022

View reviewed changes

lordcheng10 added 4 commits March 2, 2022 17:34

1. add unit test; 2. update config describe;

59a2456

check style

ac007f0

check style

8de537a

remove unused import

f843d8f

eolivelli requested changes Mar 2, 2022

View reviewed changes

remove static variable for autoShrinkForConsumerPendingAcksMap

0c285af

lordcheng10 requested review from eolivelli and Jason918 March 3, 2022 03:26

eolivelli approved these changes Mar 3, 2022

View reviewed changes

Jason918 approved these changes Mar 4, 2022

View reviewed changes

codelipenghui added this to the 2.11.0 milestone Mar 4, 2022

eolivelli merged commit e747b8f into apache:master Mar 5, 2022

codelipenghui mentioned this pull request Mar 15, 2022

[2.8.1] FGC and throw NPE #14268

Closed

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022

Optimize memory usage: support to shrink for pendingAcks map (apache#…

9abb7ae

…14515)

Technoboy- mentioned this pull request Apr 27, 2022

[improve][broker] Use shrink map for message redelivery. #15342

Merged

1 task

codelipenghui added release/2.9.3 release/2.8.4 release/2.10.1 labels Apr 27, 2022

codelipenghui pushed a commit that referenced this pull request Apr 28, 2022

Optimize memory usage: support to shrink for pendingAcks map (#14515)

aeb0929

(cherry picked from commit e747b8f)

codelipenghui added the cherry-picked/branch-2.10 label Apr 28, 2022

codelipenghui pushed a commit that referenced this pull request Apr 28, 2022

Optimize memory usage: support to shrink for pendingAcks map (#14515)

2cbf9f4

(cherry picked from commit e747b8f)

codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label Apr 28, 2022

codelipenghui pushed a commit that referenced this pull request Apr 29, 2022

Optimize memory usage: support to shrink for pendingAcks map (#14515)

a052f93

(cherry picked from commit e747b8f)

codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Apr 29, 2022

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 4, 2022

Optimize memory usage: support to shrink for pendingAcks map (apache#…

0a329ed

…14515) (cherry picked from commit e747b8f) (cherry picked from commit 2cbf9f4)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 9, 2022

Optimize memory usage: support to shrink for pendingAcks map (apache#…

aab374e

…14515) (cherry picked from commit e747b8f) (cherry picked from commit aeb0929)

Shawyeok pushed a commit to Shawyeok/pulsar that referenced this pull request Sep 6, 2022

Optimize memory usage: support to shrink for pendingAcks map (apache#…

4138c05

…14515) (e747b8f)

lhotari mentioned this pull request Nov 8, 2022

[fix][broker] fix bug caused by optimistic locking #18390

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize memory usage: support to shrink for pendingAcks map #14515

Optimize memory usage: support to shrink for pendingAcks map #14515

lordcheng10 commented Mar 1, 2022

eolivelli Mar 1, 2022

lordcheng10 Mar 1, 2022 •

edited

Loading

eolivelli Mar 1, 2022

lordcheng10 Mar 1, 2022

lordcheng10 Mar 1, 2022 •

edited

Loading

eolivelli Mar 1, 2022

lordcheng10 Mar 1, 2022

eolivelli Mar 1, 2022

lordcheng10 Mar 1, 2022 •

edited

Loading

lordcheng10 Mar 1, 2022

lordcheng10 Mar 1, 2022

lordcheng10 commented Mar 2, 2022

Jason918 Mar 2, 2022

Jason918 Mar 2, 2022

lordcheng10 Mar 2, 2022

Jason918 Mar 2, 2022

lordcheng10 Mar 2, 2022

lordcheng10 Mar 2, 2022

lordcheng10 commented Mar 2, 2022

lordcheng10 commented Mar 2, 2022

lordcheng10 commented Mar 2, 2022

eolivelli left a comment

eolivelli Mar 2, 2022

lordcheng10 Mar 2, 2022

Jason918 Mar 3, 2022

lordcheng10 Mar 3, 2022

lordcheng10 Mar 3, 2022 •

edited

Loading

lordcheng10 Mar 3, 2022 •

edited

Loading

Jason918 Mar 4, 2022

eolivelli left a comment

lordcheng10 commented Mar 4, 2022 •

edited

Loading

lordcheng10 commented Mar 4, 2022 •

edited

Loading

lordcheng10 commented Mar 5, 2022

	public static boolean autoShrinkForConsumerPendingAcksMap;
	@Getter
	private boolean autoShrinkForConsumerPendingAcksMap;

Optimize memory usage: support to shrink for pendingAcks map #14515

Optimize memory usage: support to shrink for pendingAcks map #14515

Conversation

lordcheng10 commented Mar 1, 2022

Modifications

Documentation

Choose a reason for hiding this comment

lordcheng10 Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordcheng10 Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordcheng10 Mar 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordcheng10 commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordcheng10 commented Mar 2, 2022

lordcheng10 commented Mar 2, 2022

lordcheng10 commented Mar 2, 2022

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lordcheng10 Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

lordcheng10 Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

lordcheng10 commented Mar 4, 2022 • edited Loading

lordcheng10 commented Mar 4, 2022 • edited Loading

lordcheng10 commented Mar 5, 2022

lordcheng10 Mar 1, 2022 •

edited

Loading

lordcheng10 Mar 1, 2022 •

edited

Loading

lordcheng10 Mar 1, 2022 •

edited

Loading

lordcheng10 Mar 3, 2022 •

edited

Loading

lordcheng10 Mar 3, 2022 •

edited

Loading

lordcheng10 commented Mar 4, 2022 •

edited

Loading

lordcheng10 commented Mar 4, 2022 •

edited

Loading