You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "poorbarcode (via GitHub)" <gi...@apache.org> on 2024/07/18 17:14:43 UTC
[PR] [improve] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
poorbarcode opened a new pull request, #23052:
URL: https://github.com/apache/pulsar/pull/23052
### Motivation
`LoadableCache.get` cost too many CPU resources
[broker_cpu.html.txt](https://github.com/user-attachments/files/16278265/broker_cpu.html.txt)
```
pulsar-io-4-20" #135 prio=5 os_prio=0 cpu=79326083.91ms elapsed=1465985.25s tid=0x00007f5bc40740b0 nid=0xe9 runnable [0x00007f5ba51fd000]
java.lang.Thread.State: RUNNABLE
at java.util.concurrent.ConcurrentLinkedQueue.offer(java.base@17.0.10/ConcurrentLinkedQueue.java:380)
at java.util.concurrent.ConcurrentLinkedQueue.add(java.base@17.0.10/ConcurrentLinkedQueue.java:283)
at com.google.common.cache.LocalCache$Segment.recordRead(LocalCache.java:2546)
at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2068)
at com.google.common.cache.LocalCache.get(LocalCache.java:4012)
at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:4035)
at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:5011)
at org.apache.pulsar.common.naming.TopicName.get(TopicName.java:81)
at org.apache.pulsar.common.naming.SystemTopicNames.isEventSystemTopic(SystemTopicNames.java:62)
at org.apache.pulsar.common.naming.SystemTopicNames.isSystemTopic(SystemTopicNames.java:86)
at org.apache.pulsar.common.topics.TopicList.lambda$filterSystemTopic$1(TopicList.java:65)
at org.apache.pulsar.common.topics.TopicList$$Lambda$2132/0x00007f5d049a4800.test(Unknown Source)
at java.util.stream.ReferencePipeline$2$1.accept(java.base@17.0.10/ReferencePipeline.java:178)
at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(java.base@17.0.10/ArrayList.java:1625)
at java.util.stream.AbstractPipeline.copyInto(java.base@17.0.10/AbstractPipeline.java:509)
at java.util.stream.AbstractPipeline.wrapAndCopyInto(java.base@17.0.10/AbstractPipeline.java:499)
at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(java.base@17.0.10/ReduceOps.java:921)
at java.util.stream.AbstractPipeline.evaluate(java.base@17.0.10/AbstractPipeline.java:234)
at java.util.stream.ReferencePipeline.collect(java.base@17.0.10/ReferencePipeline.java:682)
at org.apache.pulsar.common.topics.TopicList.filterSystemTopic(TopicList.java:66)
at org.apache.pulsar.broker.service.ServerCnx.lambda$handleGetTopicsOfNamespace$47(ServerCnx.java:2126)
at org.apache.pulsar.broker.service.ServerCnx$$Lambda$2129/0x00007f5d049b1c80.accept(Unknown Source)
at java.util.concurrent.CompletableFuture.uniAcceptNow(java.base@17.0.10/CompletableFuture.java:757)
at java.util.concurrent.CompletableFuture.uniAcceptStage(java.base@17.0.10/CompletableFuture.java:735)
at java.util.concurrent.CompletableFuture.thenAccept(java.base@17.0.10/CompletableFuture.java:2182)
at org.apache.pulsar.broker.service.ServerCnx.lambda$handleGetTopicsOfNamespace$49(ServerCnx.java:2123)
at org.apache.pulsar.broker.service.ServerCnx$$Lambda$2128/0x00007f5d049b1a38.apply(Unknown Source)
at java.util.concurrent.CompletableFuture.uniApplyNow(java.base@17.0.10/CompletableFuture.java:684)
at java.util.concurrent.CompletableFuture.uniApplyStage(java.base@17.0.10/CompletableFuture.java:662)
at java.util.concurrent.CompletableFuture.thenApply(java.base@17.0.10/CompletableFuture.java:2168)
at org.apache.pulsar.broker.service.ServerCnx.handleGetTopicsOfNamespace(ServerCnx.java:2120)
at org.apache.pulsar.common.protocol.PulsarDecoder.channelRead(PulsarDecoder.java:315)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.handler.flow.FlowControlHandler.dequeue(FlowControlHandler.java:202)
at io.netty.handler.flow.FlowControlHandler.channelRead(FlowControlHandler.java:164)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:346)
at io.netty.handler.codec.ByteToMessageDecoder.fireChannelRead(ByteToMessageDecoder.java:333)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:454)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:290)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:444)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.handler.flush.FlushConsolidationHandler.channelRead(FlushConsolidationHandler.java:152)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:442)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:412)
at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:440)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:420)
at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
at io.netty.channel.epoll.AbstractEpollStreamChannel$EpollStreamUnsafe.epollInReady(AbstractEpollStreamChannel.java:800)
at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:509)
at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:407)
at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:997)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.lang.Thread.run(java.base@17.0.10/Thread.java:840)
```
### Modifications
Because there is no need for a strict invalidate time, change the implementation to `ConcurrentHashMap`.
#### Performance test
After this PR:
- [test_merge_request.html.txt](https://github.com/user-attachments/files/16284759/test_merge_request.html.txt)
### 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: x
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684183874
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
It is the same
[new_test.html.txt](https://github.com/user-attachments/files/16310771/new_test.html.txt)
```java
private static final LoadingCache<String, TopicName> cache =
Caffeine.newBuilder().maximumSize(100_000).build(new CacheLoader<String, TopicName>() {
@Override
public TopicName load(String key) throws Exception {
return new TopicName(key);
}
});
```
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683744928
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,13 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
+
+ public static void clearIfReachedMaxCapacity(int maxCapacity) {
+ if (cache.size() > maxCapacity) {
+ cache.clear();
Review Comment:
The performance of `LoadingCache` is bad because it maintains some other info. You can see the flame graph in this PR.
When the cache size does not reach 100000, the performance of `ConcurrentHashMap#get` is 10x than `LoadingCache#get`.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683748328
##########
conf/broker.conf:
##########
@@ -159,6 +159,13 @@ skipBrokerShutdownOnOOM=false
# Factory class-name to create topic with custom workflow
topicFactoryClassName=
+# Max capacity of the topic name cache.
+topicNameCacheCaxCapacity=100000
+
+# A Specifies the minimum number of minutes that the system stays in the memory, to avoid clear cache frequently when
+# there are too many topics are in use.
+maxSecondsToClearTopicNameCache=7200
Review Comment:
I don't suggest adding new configurations because it requires a PIP for that.
Besides, I don't think there is much need to tune these two configs because we cannot predicate the actual use frequency of topic names.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683939904
##########
conf/broker.conf:
##########
@@ -159,6 +159,13 @@ skipBrokerShutdownOnOOM=false
# Factory class-name to create topic with custom workflow
topicFactoryClassName=
+# Max capacity of the topic name cache. 0 means unlimited cache.
Review Comment:
We'd better use -1 for unlimited cache and use 0 for no cache.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,13 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
+
+ public static void clearIfReachedMaxCapacity(int maxCapacity) {
+ if (cache.size() > maxCapacity) {
+ cache.clear();
Review Comment:
ConcurrentHashMap looks much better!
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684161645
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -625,6 +625,16 @@ public void start() throws Exception {
this.updateBrokerDispatchThrottlingMaxRate();
this.startCheckReplicationPolicies();
this.startDeduplicationSnapshotMonitor();
+ this.startClearInvalidateTopicNameCacheTask();
+ }
+
+ protected void startClearInvalidateTopicNameCacheTask() {
+ final int maxSecondsToClearTopicNameCache = pulsar.getConfiguration().getMaxSecondsToClearTopicNameCache();
+ inactivityMonitor.scheduleAtFixedRate(
+ () -> TopicName.clearIfReachedMaxCapacity(pulsar.getConfiguration().getTopicNameCacheCaxCapacity()),
+ maxSecondsToClearTopicNameCache,
+ maxSecondsToClearTopicNameCache,
+ TimeUnit.SECONDS);
Review Comment:
> This solution doesn't seem effective when the cache size can go over limits. I think it's better to use Caffeine cache with max size (as a LRU cache). You could remove expiration when using Caffeine if that's expensive.
- Clearing cache is just for the topic that was deleted, if it was not deleted, leaving it in the memory is better.
- Once there are too many topics in use, Caffeine will trigger queue items switching, which will cause more CPU costs.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684163168
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/NamespaceName.java:
##########
@@ -40,7 +40,7 @@ public class NamespaceName implements ServiceUnitId {
private final String localName;
private static final LoadingCache<String, NamespaceName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, NamespaceName>() {
+ .expireAfterWrite(30, TimeUnit.MINUTES).build(new CacheLoader<String, NamespaceName>() {
Review Comment:
Removed this change
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1685919714
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/PulsarServiceTest.java:
##########
@@ -187,6 +192,34 @@ public void testDynamicBrokerPort() throws Exception {
assertEquals(pulsar.getWebServiceAddressTls(), "https://localhost:" + pulsar.getWebService().getListenPortHTTPS().get());
}
+ @Test
+ public void testTopicCacheConfiguration() throws Exception {
+ cleanup();
+ setup();
+ assertEquals(conf.getTopicNameCacheCaxCapacity(), 5000);
+ assertEquals(conf.getMaxSecondsToClearTopicNameCache(), 5);
+
+ List<TopicName> topicNameCached = new ArrayList<>();
+ for (int i = 0; i < 20; i++) {
+ topicNameCached.add(TopicName.get("public/default/tp_" + i));
+ }
+
+ // Verify: the cache does not clear since it is not larger than max capacity.
+ Thread.sleep(10 * 1000);
+ for (int i = 0; i < 20; i++) {
+ assertTrue(topicNameCached.get(i) == TopicName.get("public/default/tp_" + i));
+ }
+
+ // Update max capacity.
+ admin.brokers().updateDynamicConfiguration("topicNameCacheCaxCapacity", "10");
+
+ // Verify: the cache were cleared.
+ Thread.sleep(10 * 1000);
+ for (int i = 0; i < 20; i++) {
+ assertFalse(topicNameCached.get(i) == TopicName.get("public/default/tp_" + i));
Review Comment:
`TopicName.get` returns `null`?
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241085367
Thanks! I’ll look later today (it’s late my time). If you are running this on Mac/Windows then reading the current time is a system call, iirc. That’s much slower than a user space like Linux, so that can artificially skew your results my not being matching the deployment destination. The ticker is can be defined if truly necessary to optimize the clock reads.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "nodece (via GitHub)" <gi...@apache.org>.
nodece commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1685916983
##########
conf/broker.conf:
##########
@@ -159,6 +159,14 @@ skipBrokerShutdownOnOOM=false
# Factory class-name to create topic with custom workflow
topicFactoryClassName=
+# Max capacity of the topic name cache. -1 means unlimited cache; 0 means broker will clear all cache
+# per "maxSecondsToClearTopicNameCache", it does not mean broker will not cache TopicName.
+topicNameCacheCaxCapacity=100000
Review Comment:
```suggestion
topicNameCacheMaxCapacity=100000
```
Typo?
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode merged PR #23052:
URL: https://github.com/apache/pulsar/pull/23052
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1685279688
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -79,11 +78,7 @@ public static TopicName get(String domain, String tenant, String cluster, String
}
public static TopicName get(String topic) {
- try {
- return cache.get(topic);
- } catch (ExecutionException | UncheckedExecutionException e) {
- throw (RuntimeException) e.getCause();
- }
+ return cache.computeIfAbsent(topic, k -> new TopicName(k));
Review Comment:
computeIfAbsent is most often pessimistic in its locking, whereas Caffeine does a lock-free read first before falling back to a computation. Since caffeine is built on top of CHM and your readers will often block each other, something seems very amiss with your benchmark results being so vastly different in performance.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684153134
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -625,6 +625,16 @@ public void start() throws Exception {
this.updateBrokerDispatchThrottlingMaxRate();
this.startCheckReplicationPolicies();
this.startDeduplicationSnapshotMonitor();
+ this.startClearInvalidateTopicNameCacheTask();
+ }
+
+ protected void startClearInvalidateTopicNameCacheTask() {
+ final int maxSecondsToClearTopicNameCache = pulsar.getConfiguration().getMaxSecondsToClearTopicNameCache();
+ inactivityMonitor.scheduleAtFixedRate(
+ () -> TopicName.clearIfReachedMaxCapacity(pulsar.getConfiguration().getTopicNameCacheCaxCapacity()),
+ maxSecondsToClearTopicNameCache,
+ maxSecondsToClearTopicNameCache,
+ TimeUnit.SECONDS);
Review Comment:
This solution doesn't seem effective when the cache size can go over limits. I think it's better to use Caffeine cache with max size (as a LRU cache). You could remove expiration when using Caffeine if that's expensive.
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/NamespaceName.java:
##########
@@ -40,7 +40,7 @@ public class NamespaceName implements ServiceUnitId {
private final String localName;
private static final LoadingCache<String, NamespaceName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, NamespaceName>() {
+ .expireAfterWrite(30, TimeUnit.MINUTES).build(new CacheLoader<String, NamespaceName>() {
Review Comment:
Why is this changed in this PR?
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241094522
> Thanks! I’ll look later today (it’s late my time). If you are running this on Mac/Windows then reading the current time is a system call, iirc. That’s much slower than a user space call like Linux, so that can artificially skew your results by not being matching the deployment destination. The ticker can be defined if truly necessary to optimize the clock reads.
@BewareMyPower do you have a chance to run the benchmark on Linux?
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2242528538
Since @ben-manes and @lhotari did not answer, let me merge it first. We can improve the code continuously in the future.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1685786473
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -79,11 +78,7 @@ public static TopicName get(String domain, String tenant, String cluster, String
}
public static TopicName get(String topic) {
- try {
- return cache.get(topic);
- } catch (ExecutionException | UncheckedExecutionException e) {
- throw (RuntimeException) e.getCause();
- }
+ return cache.computeIfAbsent(topic, k -> new TopicName(k));
Review Comment:
Answered [here](https://github.com/apache/pulsar/pull/23052#issuecomment-2241487669)
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241246130
I'm not quite sure why expiration would have such a slowdown, but it is an expensive policy. Caffeine implements it in O(1) time, but calls to check if the retrieved entry has expired is usually the dominant cost. An unbounded ConcurrentHashMap is the baseline and the cache can only try to minimize the overhead of applying the eviction policies to that.
The benchmark is single threaded, which means contention on computeIfAbsent won't be observed, and does not consume the return value, which allows for dead code elimination. These are probably not the root problem in them directly, but the full system benchmark still wouldn't make sense if you can do 5M ops/s per thread in the caches, assuming lock contention and clock sources were not a problem. Usually Guava suffers from contention (as shown in the original post), Caffeine doesn't, and if the same then it hints to the kernel clock source ([description](https://blog.packagecloud.io/system-calls-are-much-slower-on-ec2/)).
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684149229
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
It's not a fair comparison unless Caffeine is simply used as a LRU cache without expiration.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2237423473
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23052?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 73.43%. 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 [(`083afe7`)](https://app.codecov.io/gh/apache/pulsar/commit/083afe7964b87bd72c11d8e38423bd908f38d1d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 456 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/23052?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #23052 +/- ##
============================================
- Coverage 73.57% 73.43% -0.14%
- Complexity 32624 33106 +482
============================================
Files 1877 1914 +37
Lines 139502 143649 +4147
Branches 15299 15671 +372
============================================
+ Hits 102638 105495 +2857
- Misses 28908 30086 +1178
- Partials 7956 8068 +112
```
| [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23052/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/23052/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.51% <75.00%> (+2.93%)` | :arrow_up: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/23052/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.72% <75.00%> (+0.40%)` | :arrow_up: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23052/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.48% <100.00%> (-0.36%)` | :arrow_down: |
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](https://app.codecov.io/gh/apache/pulsar/pull/23052?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...org/apache/pulsar/broker/ServiceConfiguration.java](https://app.codecov.io/gh/apache/pulsar/pull/23052?src=pr&el=tree&filepath=pulsar-broker-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2FServiceConfiguration.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci1jb21tb24vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3B1bHNhci9icm9rZXIvU2VydmljZUNvbmZpZ3VyYXRpb24uamF2YQ==) | `99.02% <100.00%> (-0.37%)` | :arrow_down: |
| [...rg/apache/pulsar/broker/service/BrokerService.java](https://app.codecov.io/gh/apache/pulsar/pull/23052?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fservice%2FBrokerService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Jyb2tlclNlcnZpY2UuamF2YQ==) | `82.09% <100.00%> (+1.31%)` | :arrow_up: |
| [...org/apache/pulsar/common/naming/NamespaceName.java](https://app.codecov.io/gh/apache/pulsar/pull/23052?src=pr&el=tree&filepath=pulsar-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fcommon%2Fnaming%2FNamespaceName.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvTmFtZXNwYWNlTmFtZS5qYXZh) | `89.04% <100.00%> (ø)` | |
| [...ava/org/apache/pulsar/common/naming/TopicName.java](https://app.codecov.io/gh/apache/pulsar/pull/23052?src=pr&el=tree&filepath=pulsar-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fcommon%2Fnaming%2FTopicName.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvVG9waWNOYW1lLmphdmE=) | `96.09% <100.00%> (+0.03%)` | :arrow_up: |
... and [496 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23052/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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "ben-manes (via GitHub)" <gi...@apache.org>.
ben-manes commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241280440
The expiration is a bit of a red haring because it forces the slower path of periodic maintenance, etc. The maximum size would too, except the benchmarks set that to be 10x the populated size and tracking only begins when it is 50% full. This is support if users decide to disable the configuration using an unreachable value, e.g. `Long.MAX_SIZE`, where we shouldn't prematurely allocate the policy history (a frequency histogram). When using `initialCapacity` to force a preallocation then the timings are relatively similar between expiration and maximum size in your benchmark. On my macbook, expiration is half the throughput due to calling the system clock, but when using `ticker(Ticker.disabledTicker())` then its only 20% slower.
If adding concurrency to your tests, `@Threads(8)`, then the scaling is more pronounced on my macbook. A size-bounded cache significantly outperforms `testMapComputeIfAbsent` due to the locking (232M/s vs 14.5M/s), which is less pronounced after Java 9's improvements but can be severe if unlucky. When using expiration this drops to 6.5M/s and jumps to 121M/s if using a disabled ticker, showing that the macOS system call is the bottleneck. One could use an alternative time source, like a scheduled task to update a volatile field every second, to mimic Linux's userland approach. I think your benchmark is likely suffering the same system call overhead problem unless you can verify that it is vDSO mapped and that container virtualization is not interfering with it. Since this impacts all usages of the clock, e.g. for log events, it's probably worth checking your deployment target's behavior in case fixing the environment settings needs to be a a deployment step.
The specialized cache should certainly be faster, but its unclear if periodically clearing it would cause a latency hiccup as all requests block trying to reload the entry. Since that's unbounded and stale topics are allowed, then it sounds like expiration isn't needed and that part of the performance analysis is moot? You could use a size bounded Caffeine cache without expiration and not worry about the clock source challenges. It's all kind of hard because benchmarks don't truly reflect real world system performance, so things like soft reference caches look wonderful in isolation and fail badly in practice, so it's all a tough juggling act.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683747381
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,13 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
+
+ public static void clearIfReachedMaxCapacity(int maxCapacity) {
+ if (cache.size() > maxCapacity) {
+ cache.clear();
Review Comment:
Actually I've implemented my own `TopicName` locally. It does not use any cache, here are the test results.
```
Benchmark Mode Cnt Score Error Units
TopicNameGetBenchmark.testCustomApi thrpt 5 5218006.242 ± 110486.719 ops/s
TopicNameGetBenchmark.testLoadingCache2Get thrpt 5 6695534.090 ± 780412.049 ops/s
TopicNameGetBenchmark.testLoadingCache3Get thrpt 5 70196060.116 ± 1651256.265 ops/s
TopicNameGetBenchmark.testLoadingCacheGet thrpt 5 6437317.067 ± 600935.408 ops/s
TopicNameGetBenchmark.testPulsarApi thrpt 5 5958092.755 ± 150135.549 ops/s
```
- `testCustomApi`: Parse the topic string every time without any cache
- `testPulsarApi`: Parse the topic string only for the 1st time and read from the loading cache in the following tests
As you can see, reading from the loading cache does not perform significantly better.
<img width="1698" alt="image" src="https://github.com/user-attachments/assets/7fdf5cac-8032-4b56-9d39-133a2e6c871c">
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241080069
Here is the benchmark's source code: https://github.com/BewareMyPower/CacheBenchmark
A typical output:
```
CacheReadBenchmark.testCaffine thrpt 5 7432187.879 ± 746705.561 ops/s
CacheReadBenchmark.testGuava thrpt 5 6962764.989 ± 222299.106 ops/s
CacheReadBenchmark.testMapComputeIfAbsent thrpt 5 34219723.889 ± 686448.613 ops/s
CacheReadBenchmark.testMapGet thrpt 5 72643406.351 ± 1312709.392 ops/s
```
The overhead for expiration seems significant. @ben-manes @lhotari Please help review if there is something wrong with it.
The motivation is that the time cost of parsing a `TopicName` (with an efficient implementation, the current Pulsar implementation is inefficient) is just similar to getting the cached object from Guava `LoadingCache`.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2242559506
Good job! LGTM.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684183874
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
It is the same
[new_test.html.txt](https://github.com/user-attachments/files/16310771/new_test.html.txt)
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683987203
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
Before making this change, please compare performance to Caffeine Cache which is a successor for Guava Cache ([benchmarks](https://github.com/ben-manes/caffeine/wiki/Benchmarks)). We use Caffeine on many locations in Pulsar.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683987203
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
Before making this change, please compare performance to Caffeine Cache which is a successor for Guava Cache. We use Caffeine on many locations in Pulsar.
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684097219
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
Done, updated in the Motivation
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2237113957
/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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241487669
@ben-manes @lhotari
The implementation of Caffeine without `expireAfterWrite/expireAfterAccess` is `com.github.benmanes.caffeine.cache.SSMS`, and it depends on `ConcurrentHashMap`<sup>see pictures below</sup>. So it can never be better than `ConcurrentHashMap`.
**`get`**
<img width="988" alt="Screenshot 2024-07-21 at 13 46 30" src="https://github.com/user-attachments/assets/8562afdd-2585-49e5-89a7-27d53e4b62b1">
**`get -> load`**
<img width="893" alt="Screenshot 2024-07-21 at 13 47 43" src="https://github.com/user-attachments/assets/940ec587-bfa6-432b-b794-ed7f6ed257f5">
@BewareMyPower
Thanks for your testing, I think we'd better change the code like the one below(BTW, in my test, both implementations are close to each other)
<img width="540" alt="Screenshot 2024-07-21 at 13 52 09" src="https://github.com/user-attachments/assets/2099a951-c203-49bf-a349-5b09a43ce569">
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684183874
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
It is the same
[new_test.html.txt](https://github.com/user-attachments/files/16310771/new_test.html.txt)
```java
private static final LoadingCache<String, TopicName> cache =
Caffeine.newBuilder().maximumSize(100_000).build(new CacheLoader<String, TopicName>() {
@Override
public TopicName load(String key) throws Exception {
return new TopicName(key);
}
});
```
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "liangyepianzhou (via GitHub)" <gi...@apache.org>.
liangyepianzhou commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683716794
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,13 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
+
+ public static void clearIfReachedMaxCapacity(int maxCapacity) {
+ if (cache.size() > maxCapacity) {
+ cache.clear();
Review Comment:
Also, why can't we use a LoadingCache here?
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,13 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
+
+ public static void clearIfReachedMaxCapacity(int maxCapacity) {
+ if (cache.size() > maxCapacity) {
+ cache.clear();
Review Comment:
Shouldn't we avoid clearing all the caches?
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1685921400
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/PulsarServiceTest.java:
##########
@@ -187,6 +192,34 @@ public void testDynamicBrokerPort() throws Exception {
assertEquals(pulsar.getWebServiceAddressTls(), "https://localhost:" + pulsar.getWebService().getListenPortHTTPS().get());
}
+ @Test
+ public void testTopicCacheConfiguration() throws Exception {
+ cleanup();
+ setup();
+ assertEquals(conf.getTopicNameCacheCaxCapacity(), 5000);
+ assertEquals(conf.getMaxSecondsToClearTopicNameCache(), 5);
+
+ List<TopicName> topicNameCached = new ArrayList<>();
+ for (int i = 0; i < 20; i++) {
+ topicNameCached.add(TopicName.get("public/default/tp_" + i));
+ }
+
+ // Verify: the cache does not clear since it is not larger than max capacity.
+ Thread.sleep(10 * 1000);
+ for (int i = 0; i < 20; i++) {
+ assertTrue(topicNameCached.get(i) == TopicName.get("public/default/tp_" + i));
+ }
+
+ // Update max capacity.
+ admin.brokers().updateDynamicConfiguration("topicNameCacheCaxCapacity", "10");
+
+ // Verify: the cache were cleared.
+ Thread.sleep(10 * 1000);
+ for (int i = 0; i < 20; i++) {
+ assertFalse(topicNameCached.get(i) == TopicName.get("public/default/tp_" + i));
Review Comment:
No, it will load the second time if there is no value cached
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1684143477
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
What's the difference to Caffeine?
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241147690
@lhotari @ben-manes I ran the test on the `ubuntu-latest` GitHub Actions runner image. See results here https://github.com/BewareMyPower/CacheBenchmark/actions/runs/10020434100/job/27698014861
```
Benchmark Mode Cnt Score Error Units
CacheReadBenchmark.testCaffine thrpt 25 4895082.131 ± 308351.791 ops/s
CacheReadBenchmark.testGuava thrpt 25 4373360.896 ± 34218.258 ops/s
CacheReadBenchmark.testMapComputeIfAbsent thrpt 25 27010976.706 ± 590041.473 ops/s
CacheReadBenchmark.testMapGet thrpt 25 55345260.175 ± 462556.864 ops/s
```
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1685647755
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -54,13 +49,17 @@ public class TopicName implements ServiceUnitId {
private final int partitionIndex;
- private static final LoadingCache<String, TopicName> cache = CacheBuilder.newBuilder().maximumSize(100000)
- .expireAfterAccess(30, TimeUnit.MINUTES).build(new CacheLoader<String, TopicName>() {
- @Override
- public TopicName load(String name) throws Exception {
- return new TopicName(name);
- }
- });
+ private static final ConcurrentHashMap<String, TopicName> cache = new ConcurrentHashMap<>();
Review Comment:
Sorry, I forgot to disable `enableBrokerSideSubscriptionPatternEvaluation` when I tested with `Caffeine` before. I finished a new test and updated the result in the Motivation.
Summary:
- `with expireAfterXxx` cost more CPU resources
- `without expireAfterXxx` is close to `ConcurrentHashMap`
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "merlimat (via GitHub)" <gi...@apache.org>.
merlimat commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683904834
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -79,11 +78,9 @@ public static TopicName get(String domain, String tenant, String cluster, String
}
public static TopicName get(String topic) {
- try {
- return cache.get(topic);
- } catch (ExecutionException | UncheckedExecutionException e) {
- throw (RuntimeException) e.getCause();
- }
+ return cache.computeIfAbsent(topic, k -> {
+ return new TopicName(k);
+ });
Review Comment:
```suggestion
return cache.computeIfAbsent(topic, k -> new TopicName(k));
```
##########
conf/broker.conf:
##########
@@ -159,6 +159,13 @@ skipBrokerShutdownOnOOM=false
# Factory class-name to create topic with custom workflow
topicFactoryClassName=
+# Max capacity of the topic name cache. 0 means unlimited cache.
+topicNameCacheCaxCapacity=100000
+
+# A Specifies the minimum number of minutes that the system stays in the memory, to avoid clear cache frequently when
Review Comment:
```suggestion
# A Specifies the minimum number of seconds that the topic name stays in memory, to avoid clear cache frequently when
```
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on code in PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#discussion_r1683975311
##########
conf/broker.conf:
##########
@@ -159,6 +159,13 @@ skipBrokerShutdownOnOOM=false
# Factory class-name to create topic with custom workflow
topicFactoryClassName=
+# Max capacity of the topic name cache. 0 means unlimited cache.
Review Comment:
Maybe the following design is better:
- `-1`: unlimited cache
- `0`: broker clears all cache per `maxSecondsToClearTopicNameCache`
--
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] [broker] Improve CPU resources usege of TopicName Cache [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #23052:
URL: https://github.com/apache/pulsar/pull/23052#issuecomment-2241093670
> The overhead for expiration seems significant.
@BewareMyPower to have a proper comparison with ConcurrentHashMap, could you also run a benchmark without expiration to measure the overhead of using Caffeine as a LRU cache?
--
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