Skip to content

remove consumer unnecessary locks - #9261

Merged
codelipenghui merged 3 commits into
apache:masterfrom
hangc0276:remove_consumer_unnecessary_locks
Apr 1, 2021
Merged

remove consumer unnecessary locks#9261
codelipenghui merged 3 commits into
apache:masterfrom
hangc0276:remove_consumer_unnecessary_locks

Conversation

@hangc0276

@hangc0276 hangc0276 commented Jan 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

  1. The ConsumerImpl has many unnecessary locks for thread-safe Queue, such as Queues.newConcurrentLinkedQueue, GrowableArrayBlockingQueue, ConcurrentLinkedQueue

Changes

  1. Remove unnecessary locks in ConsumerImpl

Related to PR#8207

@315157973

315157973 commented Jan 21, 2021

Copy link
Copy Markdown
Contributor

I am interested in how much performance has improved

@hangc0276

hangc0276 commented Jan 21, 2021

Copy link
Copy Markdown
Contributor Author

I am interested in how much performance has improved

@315157973 I will paste the perf result latter.

@codelipenghui

codelipenghui commented Jan 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

@hangc0276

Remove the locks in BatchReceive APIs, with the cost of put one message more than batchReceive policy, to improve consume throughput

This will break the current behavior. In some cases, user want a guarantee of the batch size.

@codelipenghui codelipenghui reopened this Jan 21, 2021
315157973
315157973 reviewed Jan 21, 2021
View reviewed changes
Comment thread pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerBase.java Outdated
@sijie sijie added this to the 2.8.0 milestone Jan 21, 2021
@hangc0276

hangc0276 commented Jan 22, 2021

Copy link
Copy Markdown
Contributor Author

I pressure test with pulser-perf reader

1 process, 1 reader to consume 1 partition from earliest
bin/pulsar-perf read -t 1 -q 10000 -u pulsar://xxx:6650 -i 10 -m earliest persistent://test/test/xxx-partition-0

1 process, 3 reader to consume 3 partition from earliest
bin/pulsar-perf read -t 3 -q 10000 -u pulsar://xxx:6650 -i 10 -m earliest persistent://test/test/xxx-partition

The result as follow

version reader msgOut (msg/s) bytesOut (Mbit/s)
pulsar-master 3 reader 36494.152 719.944
pulsar-master 1 reader 13201.557 260.447
pulsar-master-patch 3 reader 52088.967 1027.701
pulsar-master-patch 1 reader 19219.543 379.273

It has improved 45%.
@315157973 @codelipenghui

For BatchReceive, I will test latter.

@hangc0276 hangc0276 assigned hangc0276 and unassigned 315157973 Jan 22, 2021
@hangc0276

hangc0276 commented Jan 24, 2021

Copy link
Copy Markdown
Contributor Author

For BatchReceive, I add batchReceive policy for PerformanceConsumer #9295 , and test as follow

  1. create topic with 30 partitions (persistent://test/test/test_batch_v1)
  2. run perf producer at high performance
  3. run consumer with batch receive policy and start from latest to ensure read message from cache, in order to avoid read bookie entry disk

perf command as follow:

  1. run perf producer
bin/pulsar-perf produce -u pulsar://xxx:6650 -i 10 -r 500000 -threads 5 -ioThreads 3 persistent://test/test/test_batch_v1
  1. run consumer using pulsar-master lib and pulsar-master-patch lib
bin/pulsar-perf consume -u pulsar://xxx:6650 -i 10 -sp Latest -time 240 -s batch-perf-v1 -q 10000 -bb 1048576 -bm 100 -bt 100 persistent://test/test/test_batch_v1

The result as follow

version msgOut (msg/s) bytesOut (Mbit/s)
pulsar-master 110081.956 860.015
pulsar-master-patch 116298.462 908.582

The consume performance improved 5.6%
The pulsar-master-patch version just remove the lock for batchReceive module.

I will move the lock back for batchReceive module.
@codelipenghui @315157973

@hangc0276

hangc0276 commented Jan 25, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@hangc0276 hangc0276 closed this Jan 25, 2021
@hangc0276 hangc0276 reopened this Jan 25, 2021
@hangc0276
hangc0276 force-pushed the remove_consumer_unnecessary_locks branch from 516a9d5 to 99f07fa Compare January 25, 2021 02:43
@hangc0276

hangc0276 commented Jan 25, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

5 similar comments
@hangc0276

hangc0276 commented Jan 25, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@hangc0276

hangc0276 commented Jan 26, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@hangc0276

hangc0276 commented Jan 26, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@hangc0276

hangc0276 commented Jan 26, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@hangc0276

hangc0276 commented Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@315157973

315157973 commented Feb 1, 2021

Copy link
Copy Markdown
Contributor

I checked where the lock is removed, it should be thread safe

cc @eolivelli

eolivelli
eolivelli approved these changes Feb 1, 2021
View reviewed changes

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

cc @sijie @merlimat @rdhabalia PTAL

@sijie

sijie commented Feb 2, 2021

Copy link
Copy Markdown
Member

@codelipenghui Can you review this again?

@hangc0276

hangc0276 commented Feb 3, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@hangc0276

hangc0276 commented Feb 3, 2021

Copy link
Copy Markdown
Contributor Author

/pulsarbot run-failure-checks

@hangc0276
hangc0276 force-pushed the remove_consumer_unnecessary_locks branch from 909fe40 to 6f21173 Compare March 28, 2021 09:26
@hangc0276
hangc0276 requested a review from merlimat March 31, 2021 02:21
@hangc0276

hangc0276 commented Mar 31, 2021

Copy link
Copy Markdown
Contributor Author

@codelipenghui @sijie @merlimat PTAL again, thanks.

@codelipenghui

codelipenghui commented Mar 31, 2021

Copy link
Copy Markdown
Contributor

@hangc0276 I think the description should be updated since the change does not break the batch receive behavior?

@hangc0276

hangc0276 commented Mar 31, 2021

Copy link
Copy Markdown
Contributor Author

@hangc0276 I think the description should be updated since the change does not break the batch receive behavior?

@codelipenghui Ok, i have updated the description.

@eolivelli
eolivelli requested a review from 315157973 March 31, 2021 14:55
@codelipenghui
codelipenghui merged commit 9d08f64 into apache:master Apr 1, 2021
@315157973 315157973 mentioned this pull request Apr 23, 2021
Merged
linlinnn pushed a commit to linlinnn/pulsar that referenced this pull request Apr 26, 2021
linjunhua
Revert "Remove consumer unnecessary locks (apache#9261)"
0b8288e 
This reverts commit 9d08f64
linlinnn pushed a commit to linlinnn/pulsar that referenced this pull request Apr 27, 2021
linjunhua
Revert "Remove consumer unnecessary locks (apache#9261)"
a7aba84 
This reverts commit 9d08f64
codelipenghui pushed a commit that referenced this pull request Apr 27, 2021
@linlinnn
[consumer] Revert "Remove consumer unnecessary locks (#9261)" (#10240)
e95d6f0
@codelipenghui codelipenghui mentioned this pull request May 7, 2021
Merged
sijie pushed a commit that referenced this pull request May 8, 2021
@codelipenghui
Move out the key_shared related tests from the quarantine group. (#10508
5e56503 
)

### Motivation

#10240 has reverted the changes of the #9261 introduced which make the key_shared tests flaky. So it's better to move out the tests from the quarantine group.

### Modifications

Move out the key_shared related tests from the quarantine group.
@hangc0276 hangc0276 mentioned this pull request May 11, 2021
Closed
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request May 11, 2021
@codelipenghui @eolivelli
Move out the key_shared related tests from the quarantine group. (apa…
0bd4e8c 
…che#10508)

### Motivation

apache#10240 has reverted the changes of the apache#9261 introduced which make the key_shared tests flaky. So it's better to move out the tests from the quarantine group.

### Modifications

Move out the key_shared related tests from the quarantine group.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eolivelli eolivelli eolivelli approved these changes
@315157973 315157973 315157973 approved these changes
@codelipenghui codelipenghui codelipenghui approved these changes
@jiazhai jiazhai Awaiting requested review from jiazhai
@sijie sijie Awaiting requested review from sijie
@merlimat merlimat Awaiting requested review from merlimat

Assignees

@hangc0276 hangc0276

Labels

area/client

Projects

None yet

Milestone

2.8.0

Development

Successfully merging this pull request may close these issues.

5 participants

@hangc0276 @315157973 @codelipenghui @sijie @eolivelli