Replace RwLock<HashMap> and Mutex<HashMap> by using DashMap

[yahoNanJing]

Which issue does this PR close?

Closes #4077 .

Rationale for this change

DashMap leverages shard-level lock to achieve good performance for high concurrency, like the ConcurrencyHashMap in Java.

What changes are included in this PR?

Are there any user-facing changes?

[tustvold]

I'm surprised any of these data structures are contended, and so dashmap is unlikely to yield benefits here? It may even be slower.

Do you have any benchmark results you could share?

[alamb]

I agree I think it would be good to have some more evidence that using a new third-party library adds benefits -- it would be interesting to know more about the experience in Ballista.

[alamb]

Or maybe put another way, if a different locking implementation improved performance significantly in Ballista, I wonder if we should investigate ways to avoid locking all together (copy-on-write, for example) 🤔

[alamb]

Looks like maybe dashmap was added in apache/datafusion-ballista#319 but there aren't any performance measurements that I could see there

[metesynnada]

I'm surprised any of these data structures are contended, and so dashmap is unlikely to yield benefits here? It may even be slower.

Do you have any benchmark results you could share?

Actually, it is better in most use cases. You can check . It is quite similar to Java's ConcurrentHashMap.

[tustvold]

Those benchmarks are all for high throughput workloads, in this case I would be extremely surprised to see these hashmaps showing up in profiles. In the absence of a compelling benchmark it is hard for me to approve this... It is a non-trivial additional dependency, not to mention one that I've run into API issues with in the past, for an unclear benefit

[yahoNanJing]

Thanks @alamb and @tustvold, I'll try to add some benchmark testing before this PR be accepted.

[xudong963]

I'm not sure if it'll improve performance, but the code looks cleaner!

[ozankabak]

My two cents: I think it is more about encapsulating certain low-level operations regarding the data structure within the data structure itself, vs. exposing them in higher-level code. I think this is probably what @xudong963 means by cleaner, and I agree.

I also agree with @tustvold that typical workloads will not result in contention, without which performance improvements will not manifest.

If this change would merge, the benefits would be:

1. Code with better separation of concern (although the benefit is not as much as the Ballista change)
2. Potential performance gain in the unrealistic-looking contention scenarios
3. Being more in-line with Ballista codebase

The main disadvantage that I can see:

1. Introducing a dependency to Datafusion (albeit it is fairly commonly-used one)

I don't think the argument either way is super strong, but it seems to me that pros outweigh the cons (unless I am missing something).

[alamb]

I agree this makes the code look cleaner and I will defer to your judgement that the pros outweigh the cons. It looks like this just needs to have the datafusion-cli Cargo.lock file updated and it should be good to go

[alamb]

Until the fix for #4100 is merged, clippy will be failing on this PR as well

[ozankabak]

@yahoNanJing, given that #4100 is merged, this can merge if you resolve the conflicts.

[yahoNanJing]

Thanks @ozankabak. Just rebased.

[xudong963]

Thanks @yahoNanJing

[alamb]

Let's plan to merge this tomorrow if we don't hear any additional comments 👍

[alamb]

Thanks again everyone!

[ursabot]

Benchmark runs are scheduled for baseline = 4d23cae and contender = a9add0e. a9add0e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java