[improve][broker] Improve the performance of TopicName

[BewareMyPower]

Motivation

The TopicName's constructor has poor performance:

• NamespaceName#get is very slow
• Splitter.on("/").splitToList(rest) is slow
• String.format is slower than the + operation on strings and completeTopicName is unnecessarily created again

Modifications

• Initialize NamespaceName in a lazy way (don't do that because it assumes the constructor is called more frequently than getNamespace or getNamespaceObject)
• Replace Splitter with a manually written splitBySlash introduced from [fix][proxy] Fix proxy OOM by replacing TopicName with a simple conversion method #24465. Actually, StringUtils#split has good performance as well. But it will split "//xxx/yyy/zzz" to ["xxx", "yyy", "zzz"] without reporting any error.
• Initialize completeTopicName from the argument directly without any concentrate operation
• Replace String.format with + in fromPersistenceNamingEncoding

Documentation

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

Matching PR in forked repository

PR in forked repository: BewareMyPower#44

[BewareMyPower]

Before 48ffb7a, there is a constructor parameter that determines whether to initialize the NamespaceName. I also compared with the existing TopicName constructor, which can be found here: https://gist.github.com/BewareMyPower/6b83c897552c110f336e51965cc91c24

The benchmark result is:

TopicNameBenchmark.testConstruct                          thrpt       312.983          ops/s
TopicNameBenchmark.testConstructWithNamespaceInitialized  thrpt       101.973          ops/s
TopicNameBenchmark.testLegacyTopicName                    thrpt        53.944          ops/s
TopicNameBenchmark.testReadFromCache                      thrpt       721.392          ops/s

As you can see

• Accessing it from the cache is only 2.3x faster than the latest implementation, which is not significant. However, to avoid debate on whether to remove cache in this PR, I only exposed the public constructor.
• Skipping initialization of NamespaceName is 3x faster than initializing it immediately
• It's 6x faster than the original implementation. Even if initializing NamespaceName immediately, it's still about 2x faster.

[lhotari]

Before 48ffb7a, there is a constructor parameter that determines whether to initialize the NamespaceName. I also compared with the existing TopicName constructor, which can be found here: https://gist.github.com/BewareMyPower/6b83c897552c110f336e51965cc91c24

The benchmark result is:

TopicNameBenchmark.testConstruct                          thrpt       312.983          ops/s
TopicNameBenchmark.testConstructWithNamespaceInitialized  thrpt       101.973          ops/s
TopicNameBenchmark.testLegacyTopicName                    thrpt        53.944          ops/s
TopicNameBenchmark.testReadFromCache                      thrpt       721.392          ops/s

As you can see

• Accessing it from the cache is only 2.3x faster than the latest implementation, which is not significant. However, to avoid debate on whether to remove cache in this PR, I only exposed the public constructor.
• Skipping initialization of NamespaceName is 3x faster than initializing it immediately
• It's 6x faster than the original implementation. Even if initializing NamespaceName immediately, it's still about 2x faster.

I wouldn't trust JMH results from benchmarking on Mac OS. In the case of PR #24457, the results are very different when benchmarking on Linux x86_64 with Intel i9 processor (Dell XPS 2019 laptop).
If we'd completely remove the cache, there would be more duplicate TopicName and NamespaceName instances and more duplicate string instance. For very long tenant, namespace and topic names, that could add up a significant amount of wasted heap memory.

[BewareMyPower]

@lhotari So could you help run benchmark in your Linux environment to see the difference? I can also create a workflow via GitHub Actions to see the result.

[BewareMyPower]

If we'd completely remove the cache, there would be more duplicate TopicName and NamespaceName instances and more duplicate string instance. For very long tenant, namespace and topic names, that could add up a significant amount of wasted heap memory.

I don't want to debate about it in this PR. If you have a chance to look at how TopicName is used in Pulsar, you will find a lot of temporary TopicName instances just for conversion. I know near all Pulsar constributors are very sensitive to the topic about GC, so I don't change the TopicName#get implementation.

The ultimate solution is to create some utils methods instead of leveraging the TopicName class.

[BewareMyPower]

Updated test results to compare it with the legacy implementation in https://github.com/BewareMyPower/pulsar/actions/runs/15872434081/job/44752001433?pr=45

TopicNameBenchmark.testConstruct                             thrpt       258.932          ops/s
TopicNameBenchmark.testConstructWithNamespaceInitialization  thrpt       158.876          ops/s
TopicNameBenchmark.testLegacyConstruct                       thrpt        44.290          ops/s
TopicNameBenchmark.testReadFromCache                         thrpt       340.613          ops/s

• The constructor is 5.8x faster than the existing one
• The constructor with NamespaceName initialization is 3.6x faster than the existing one

[BewareMyPower]

@lhotari @codelipenghui @nodece @dao-jun @poorbarcode @coderzc This PR is now ready to review, PTAL

[merlimat]

We shouldn't need to have namespaceName volatile, we can just check it again in the synchronized block.

[BewareMyPower]

But could it have the issue with double-checked locking?

The code generated by the compiler is allowed to update the shared variable to point to a partially constructed object before A has finished performing the initialization.

[lhotari]

But could it have the issue with double-checked locking?

That's true, there's a good explanation of the problem in https://shipilev.net/blog/2014/safe-public-construction/
The double-checked locking pattern isn't recommended in general since it could lead to a situation where the shared instance seems to be partially instantiated, unless it's completely immutable (this applies to possibly created contained instances in the constructor). In Java "benign data races" don't lead to crashes, but to potential consistency issues where the state of fields are inconsistent.

Perhaps in this case, double checked locking would be possible assuming that NamespaceName is completely immutable in regards to fields set in the constructor?

[lhotari]

Perhaps in this case, double checked locking would be possible assuming that NamespaceName is completely immutable in regards to fields set in the constructor?

According to https://shipilev.net/blog/2014/safe-public-construction/, it wouldn't be safe. There isn't really a way around it.

[lhotari]

Safe publication makes all the values written before the publication visible to all readers that observed the published object. It is a great simplification over the JMM rules of engagement with regards to actions, orders and such.
There are a few trivial ways to achieve safe publication:

• Exchange the reference through a properly locked field (JLS 17.4.5)

• Use static initializer to do the initializing stores (JLS 12.4)

• Exchange the reference via a volatile field (JLS 17.4.5), or as the consequence of this rule, via the AtomicX classes

• Initialize the value into a final field (JLS 17.5).

[lhotari]

Well in this case, since we aren't constructing NamespaceName but instead calling NamespaceName.get (which handles thread safety with the cache implementation), it would be possible to avoid using a volatile field and just use DCL.

[BewareMyPower]

But the thread safety depends on the implementation of NamespaceName.get. It would also be dangerous in case someone else did copy-and-paste in future.

I decided to remove the lazy initialization for now. The prerequisite of the performance improvement is that the TopicName's constructor is called more frequently than the getNamespace or getNamespaceObject methods.

Let's use a conservative solution for now

[lhotari]

@BewareMyPower My point from an earlier comment isn't addressed:

If we'd completely remove the cache, there would be more duplicate TopicName and NamespaceName instances and more duplicate string instance. For very long tenant, namespace and topic names, that could add up a significant amount of wasted heap memory.

In this case, I think it's irrelevant to just benchmark the performance of creating/looking up TopicName instances.

Duplicate java.lang.String instances could increase significantly after introducing a non-caching solution. Heapdump analysis tools have such checks for duplicate instances so that's one way how to compare the difference.

I do agree that a lot of the TopicName handling code is a mess. For example in Topic listing, the topic name is converted from/to String multiple times. That's adding up a lot of pressure for a fast lookup solution when instances are cached. So I'm not against changing the current caching, it's just that there's a need to consider duplicate instances as well. It's likely that the caching is more relevant for NamespaceName than TopicName regarding duplicate instances. We might not be keeping a reference to TopicName instance in that many places when broker is running.

[BewareMyPower]

If we'd completely remove the cache,

I've read more code and changed my idea a bit. Caching could be helpful in many cases. But how to establish the cache might depend on specific use cases. Writing a common cache is hard. I don't like the solution in #23052, but it's anyway good to resolve the issue encountered at that time.

As I've mentioned here, exposing the public method is helpful for downstream to construct its own cache. In addition, the TopicName can be cached as well as some other classes, e.g. Map<String, Pair<TopicName, PersistentTopic>> would be better than maintaining two maps:

• A built-in Map<String, TopicName>
• An external Map<String, PersistentTopic>

It's just an example, PersistentTopic can be replaced by another topic abstraction class.

[BewareMyPower]

Regarding this PR, I'm going to revert other changes and only leaving the improvement on TopicName's constructor itself.

Exposing a public method will be easier for the downstream to maintain its custom cache, but it will also be confusing for the core Pulsar developers to make the decision on TopicName.get and new TopicName.

Anyway, we should improve the use of TopicName case by case. Take BrokerService for example, a TopicName is passed to getTopic, but the string returned by toString() is passed to

• loadOrCreatePersistentTopic
• TopicLoadingContext#topic

The TopicName instance is constructed again by them to checkOwnershipAndCreatePersistentTopic.

In this case, TopicName#get makes sense than new TopicName. But the code is still inefficient, we should not perform the unnecessary topicName.toString() -> TopicName.get(topic) conversions.

[lhotari]

This PR will be an improvement. Regarding the argument I made in a previous comment about duplicate tenant and namespace java.lang.String instances, that problem is already present in the current code base. That could be solved in a different PR.

[lhotari]

LGTM

[lhotari]

For deduplicating the tenant and namespace String instances, it would be useful to assign the tenant and namespacePortion fields from the NamespaceName instance. This wouldn't add much overhead, but benefit in reducing the amount of heap memory since there would be less duplication of java.lang.String instances at runtime.