[SPARK-26155] Optimizing the performance of LongToUnsafeRowMap

[LuciferYang]

What changes were proposed in this pull request?

To slove @JkSelf report problem at SPARK-26155, use LongAdder instead of Long of numKeyLookups and numProbes to reduce add operation times. @JkSelf test this patch in Intel performance testing environment and run TPCDS sqls after this patch with Spark-2.3 and master no longer slower than Spark-2.1.

How was this patch tested?

N/A

[LuciferYang]

cc @JkSelf help to check this patch.

[LuciferYang]

cc @cloud-fan , help to review this patch?

[LuciferYang]

ping @viirya

[adrian-wang]

maybe add some detailed test result in description and explain the reason for this in code comment?

[JkSelf]

@LuciferYang the patch is fine in my test environment.
@adrian-wang I will run all the tpcds queries in spark2.3 and spark2.3 with this patch later.

[LuciferYang]

@JkSelf thx~

[LuciferYang]

@adrian-wang ok~ I will add some comments to explain the reason

[cloud-fan]

ok to test

[cloud-fan]

I'm surprised. I think LongToUnsafeRowMap is used in a single thread environment and multi-thread contend should not be an issue here. Do you have any insights about how this fixes the perf issue?

[LuciferYang]

Initially, I thought these two variables in class scope will affect SIMD optimization of JIT(after java8), we try to add -XX: -UseSuperWord to executor java opts to vertify this view, but no affect with spark-2.1, although this patch can improve performance....

[cloud-fan]

I might know the root cause: LongToUnsafeRowMap is acutally accessed by multiple threads.

For broadcast hash join, we will copy the broadcasted hash relation to avoid multi-thread problem, via HashedRelation.asReadOnlyCopy. However, this is a shallow copy, the LongToUnsafeRowMap is not copied and likely shared by multiple HashedRelations.

The metrics is per-task, so I think a better fix is to track the hash probe metrics per HashedRelation, instead of per LongToUnsafeRowMap. It's too costly to copy the LongToUnsafeRowMap, we should think about how to do it efficiently. cc @hvanhovell

[cloud-fan]

It's easy to track numKeyLookups at HashedRelation, but it's hard to track numProbes. One idea is, we pass a MutableInt to LongToUnsafeRowMap.getValue as a parameter, and in the method we set the actual numProbes of this look up to the MutableInt parameter.

[SparkQA]

Test build #99651 has finished for PR 23214 at commit a267e6b.

• This patch fails due to an unknown error code, -9.
• This patch merges cleanly.
• This patch adds no public classes.

[viirya]

Thanks for doing this. I think we are more close to the root cause.

[LuciferYang]

For broadcast hash join, we will copy the broadcasted hash relation to avoid multi-thread problem, via HashedRelation.asReadOnlyCopy. However, this is a shallow copy, the LongToUnsafeRowMap is not copied and likely shared by multiple HashedRelations.

Was there no problems of data correctness in the past use unthread-safe Long type?

[cloud-fan]

I think there is a problem, but no one found out because it's only about metrics.

[LuciferYang]

On the other hand, if is only a multi-thread problem, may not affect performance because there is no synchronized code part ...

[LuciferYang]

As @cloud-fan said the hash join metrics is wrongly implemented, we will partial revert # SPARK-21052, no longer need this patch, close it ~