[SPARK-25317][CORE] Avoid perf regression in Murmur3 Hash on UTF8String

[mgaido91]

What changes were proposed in this pull request?

SPARK-10399 introduced a performance regression on the hash computation for UTF8String.

The regression can be evaluated with the code attached in the JIRA. That code runs in about 120 us per method on my laptop (MacBook Pro 2.5 GHz Intel Core i7, RAM 16 GB 1600 MHz DDR3) while the code from branch 2.3 takes on the same machine about 45 us for me. After the PR, the code takes about 45 us on the master branch too.

How was this patch tested?

running the perf test from the JIRA

[viirya]

So seems the performance regression is due to the cost of virtual function calls on MemoryBlock?

[mgaido91]

that was my guess too at the beginning, but if you just do this change, performance won't change. Seems reasonable what said by @kiszk about the clue being the size of the javabyte code generated, but needs more investigation.

[viirya]

Ok. Seems there are more than single cause for this performance regression.

[mgaido91]

cc @cloud-fan@kiszk

I checked the bytecode generated and the size of the generated code seems not to be the issue either. FYI I am attaching here the disassembled code before and after:
afterPatch.txt
beforePatch.txt

[cloud-fan]

Thanks for working on it! Will it be helpful if we move these hash methods to MemoryBlock? e.g. the code can be int halfWord =bytes[offset + i];

[mgaido91]

@cloud-fan I think I tried doing something like what you suggested but it didn't help. Moreover, the current code in MemoryBlock already leverages Platform.get* for most of its methods, so that wouldn't really change much I think.

[cloud-fan]

This basically reverts the memory block in the hash computing, now the memory block is just a holder of the base object and base offset. This does fix the regression, will we also lose the perf speed up too? IIUC we did observe significant perf boost when introducing the memory block.

[SparkQA]

Test build #95704 has finished for PR 22338 at commit 91adce5.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[kiszk]

@mgaido91 thanks, interestingly I did experiments with similar code in my box.
While I am using the linux box, I can confirm the performance improvement (or performance recover).

Regarding bytecode size, what I have seen in my experiments is the following. I have not found the root cause of this behavior. Is checkedCast too slow, which takes 74us?

Your original code: 38us

 public static int hashUnsafeBytesBlock(MemoryBlock base, int seed) {
return hashUnsafeBytesBlock(base, Ints.checkedCast(base.size()), seed); }
private static int hashUnsafeBytesBlock(MemoryBlock base, int lengthInBytes, int seed) {
// This is not compatible with original and another implementations. // But remain it for backward compatibility for the components existing before 2.3. assert (lengthInBytes >= 0): "lengthInBytes cannot be negative";
int lengthAligned = lengthInBytes - lengthInBytes % 4;
....

Your original code with moving checkedCast: 112us

 public static int hashUnsafeBytesBlock(MemoryBlock base, int seed) {
// return hashUnsafeBytesBlock(base, Ints.checkedCast(base.size()), seed); return hashUnsafeBytesBlock(base, -1, seed);
}
private static int hashUnsafeBytesBlock(MemoryBlock base, int lengthInBytes, int seed) {
// This is not compatible with original and another implementations. // But remain it for backward compatibility for the components existing before 2.3. lengthInBytes = Ints.checkedCast(base.size()); // MOVED checkedCast() here
assert (lengthInBytes >= 0): "lengthInBytes cannot be negative";
int lengthAligned = lengthInBytes - lengthInBytes % 4;
...

[mgaido91]

@kiszk yes, I know. Moving checkedCast makes a big difference, but if you just move it, without the other changes, there is no perf gain (at least this is what I found in my experiments).

[kiszk]

In addition to your commit, I applied the following change, basically use MemoryBlock in hashBytesByIntBlock(). I got 33us.

 private static int hashBytesByIntBlock(MemoryBlock base, int lengthInBytes, int seed) {
assert (lengthInBytes % 4 == 0);
int h1 = seed;
for (int i = 0; i < lengthInBytes; i += 4) {
int halfWord = base.getInt(i);
int k1 = mixK1(halfWord);
h1 = mixH1(h1, k1);
}
return h1;
}

But, furthermore, when I also used MemoryBlock in hashUnsafeBytesBlock(), I got 111us.

[mgaido91]

This does fix the regression, will we also lose the perf speed up too? IIUC we did observe significant perf boost when introducing the memory block.

@cloud-fan I checked the original PR and I couldn't find any benchmark related to Murmur3 hash. The only benchmark I found there was using the HiveHasher. So I cannot really answer on this. Do you have a benchmark to test it? Thanks.

[mgaido91]

you're right @kiszk. Let me update the PR accordingly in order to narrow down the change/problem, thanks.

[kiszk]

While I saw these performance differences, I do not understand why these difference occurs completely. That is why I said "I have not found the root cause".

Let us narrow down the problem and find the root cause.

[mgaido91]

While I say these performance differences, I do not understand why these difference occurs completely. That is why I said "I have not found the root cause".

Yes, I am in the same situation. I see the difference but I cannot explain it.

[viirya]

Yeah, it's interesting... Seems both checkedCast and Platform.getByte are changed and then performance gets gain. Any single change doesn't work.

[SparkQA]

Test build #95721 has finished for PR 22338 at commit 6cb94ed.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[cloud-fan]

Since the change looks safer to me and it does fix the regression, I'm merging it to unblock 2.4 release. Please continue to investigate the root cause, thanks!