PR list

• [fix][broker] Fix thread unsafe access on the bundle range cache for load manager #23217
• [improve][broker] Replace ConcurrentOpenHashMap with ConcurrentHashMap in BrokerService #23320
• [improve][broker] Replace ConcurrentOpenHashMap with ConcurrentHashMap in Topic classes #23322
• [improve][broker] Remove ConcurrentOpenHashMap and ConcurrentOpenHashSet #23329

Search before asking

• I searched in the issues and found nothing similar.

Motivation

There was a discussion at Sep. 2023 before to replace Customized Map with ConcurrentOpenHashMap. In this issue, I'd focus on the ConcurrentOpenHashMap.

Here is the only advantage of this map.

Provides similar methods as a {@code ConcurrentMap<K,V>} but since it's an open hash map with linear probing, no node allocations are required to store the values.

Let me count the disadvantages.

1. Bad performance

This map was aded in the initial commit (on 2016). However, this implementation was just based on the Java 7 implementation of the ConcurrentHashMap, which uses a segment based lock. Actually, this solution was discarded by the Java team since Java 8.

#20647 did a benchmark and found the performance was much worse than the current ConcurrentHashMap provided by Java library. We can also search the PROs of the Java 8 design in network, or just ask for ChatGPT.

Besides, the frequently used keys() and values() methods just copy the keys and values to a new list. While the ConcurrentHashMap just returns a thread-safe internal view that users can choose whether to make a copy.

Anyway, to prove the performance is worse than ConcurrentHashMap, we need to have more tests and research. So it's the least important reason.

2. Lack of the updates

This class was rarely updated. What I can remember is the shrink support two years ago. #14663

From apache/bookkeeper#3061, we can see the motivation is the frequently appeared Full GC caused by this implementation. However, adding a shrink method makes it harder to use. There are already many parameters to tune, see it's builder:

    public static class Builder<K, V> {
        int expectedItems = DefaultExpectedItems;
        int concurrencyLevel = DefaultConcurrencyLevel;
        float mapFillFactor = DefaultMapFillFactor;
        float mapIdleFactor = DefaultMapIdleFactor;
        float expandFactor = DefaultExpandFactor;
        float shrinkFactor = DefaultShrinkFactor;
        boolean autoShrink = DefaultAutoShrink;

Many xxxFactors and the concurrency level. It's hard to determine a proper value by default. However, it makes new developers hard to modify it.

3. Bad debug experience

When I debugged the topics maintained in a BrokerService.

As you can see. There are 16 sections. And I have to iterate over all these sections and expand the table array to find the target topic.

Let's compare it with the official ConcurrentHashMap (I replaced it locally)

Besides, it's even harder to analyze in the heap dump.

4. Not friendly to new developers

Many places just use it as a concurrent hash map. What's the reason for new developers to not use the official ConcurrentHashMap, which is developed and consistently improved by a professional team? Just to reduce the node allocation? With the improving JVM GC?

As I've mentioned, this class might be introduced at the Java 7 era. Now the minimum required Java version of broker side is 17. We have ZGC. We have Shenandoah GC. We have many more JVM developers developing better GC. I'm suspecting if the advantage makes sense.

I cannot think of a reason to choose this hard-to-maintain class rather than well-maintained official ConcurrentHashMap.

For example, when I maintained KoP, I encountered the deadlock of ConcurrentLongHashMap (maybe the similar implementation). streamnative/kop#620 And it's hard to know if this case is fixed. So I have to switch to the official ConcurrentHashMap.

Solution

Replace ConcurrentOpenHashMap with the official Java ConcurrentHashMap.

Alternatives

N/A

Anything else?

Java Concurrent HashMap Improvements over the years https://medium.com/@vikas.taank_40391/java-concurrent-hashmap-improvements-over-the-years-8d8b7be6ce37

Are you willing to submit a PR?

• I'm willing to submit a PR!