Hi, Guys. IMO, It's a data structure difference. We can give the benchmark of two structures in different situations (pulsar using situations). > we can built a distribution for the benchmark. After we get the result, > then we can decide if it's fine to go to the new solution. The change should > also be > applied to bookkeeper. And I can help to run the benchmark comparison by using > openmessaging benchmark and share the result here. Hi, Penghui
I am not sure if deserve to spend many time on distribution testing. but I think other contributors would love it If we can get some comparison of two different data structure. Best, Mattison On 6 Sep 2023 at 22:00 +0800, PengHui Li <peng...@apache.org>, wrote: > I also support the idea. > > For the execution. I think we can have a branch to apply the changes and > then > we can built a distribution for the benchmark. After we get the result, > then we can > decide if it's fine to go to the new solution. The change should also be > applied to > bookkeeper. And I can help to run the benchmark comparison by using > openmessaging > benchmark and share the result here. > > Regards, > Penghui > > On Wed, Sep 6, 2023 at 4:56 PM Yunze Xu <x...@apache.org> wrote: > > > I support replacing it with JDK's ConcurrentHashMap. Maintaining a > > customized concurrent hash map whose algorithm is essentially the same > > with the implementation of a very old version of JDK is painful. The > > PROs listed like no boxing and linear probing have not proved to be > > better by any benchmark. Instead, Penghui's benchmark shows the > > performance is much worse. > > > > Regarding the remove or other operations in forEach method, I believe > > there is a good way to resolve it and we should not do many things in > > the forEach callback. > > > > Thanks, > > Yunze > > > > On Wed, Sep 6, 2023 at 2:36 PM the tumbled <wof...@qq.com.invalid> wrote: > > > > > > > > > > > > > > > > On 2023/09/06 04:22:58 QQ wrote: > > > > > > Hi Pulsar Community, > > > > > > > > > > > > I’d like to start a discussion about whether replacing the customize > > util class like ConcurrentOpenHashMap with ConcurrentHashMap, as the > > performance of ConcurrentHashMap is better than those customize util > > significantly. > > > > > > Worse, these customize util class cannot ensure consistence in > > > > > > method > > forEach as PR https://github.com/apache/pulsar/pull/21110 shows, which is > > disquieting, although it may not cause any problem. > > > > > > > > > > > > Thanks, > > > > > > The Tumbled. > > > > > > > > > > > > > > The benchmark results is provided in > > https://github.com/apache/pulsar/pull/20647\#issuecomment-1607257960 < > > https://github.com/apache/pulsar/pull/20647%5C#issuecomment-1607257960>. > > > > > > > > > > > > > > However, iterators are designed to be used by only one thread at a > > time. Bear in mind that the results of aggregate status methods including > > size, isEmpty, and containsValue are typically useful only when a map is > > not undergoing concurrent updates in other threads. Otherwise the results > > of these methods reflect transient states that may be adequate for > > monitoring or estimation purposes, but not for program control. > > > > > > > > And I notice that ConcurrentHashMap’s forEach method do not support > > thread-safety too. > > > > It seems that the only reason left for us to replacing the customized > > util is the superior performance of ConcurrentHashMap. > > > > > > > > Thanks, > > > > The Tumbled. > >
