Refactor MD5 implementation. #9212

bdice · 2021-09-10T03:10:24Z

This PR refactors the MD5 hash implementation in libcudf. I used the MD5 code as a reference while working on SHA (extending #6020, PR #9215 to follow).

List of high-level changes:

I moved the implementation of MD5Hash and related logic from include/cudf/detail/utilities/hash_functions.cuh to src/hash/md5_hash.cu because it is only used in that file and nowhere else. We don't need to include and build MD5 in hash_functions.cuh for all the collections/sorting/groupby tools that only use Murmur3 variants and IdentityHash. (This will be a bigger deal once we add the SHA hash functions, soon to follow this PR, because the size of hash_functions.cuh would be substantially larger without this separation.)
I removed an MD5Hash constructor that accepted and stored a seed whose value was unused.
Improved use of namespaces.
Use named constants instead of magic numbers.
Introduced a hash_circular_buffer and refactored dispatch logic.

No changes were made to the feature scope or public APIs of the MD5 feature, so existing unit tests and bindings should remain the same.

…se they are only used in that file.

bdice · 2021-09-14T17:03:51Z

rerun tests

codecov · 2021-09-14T18:46:19Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.12@4349232). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head fa7321c differs from pull request most recent head c7d132f. Consider uploading reports for the commit c7d132f to get more accurate results

@@               Coverage Diff               @@
##             branch-21.12    #9212   +/-   ##
===============================================
  Coverage                ?   10.75%           
===============================================
  Files                   ?      116           
  Lines                   ?    19480           
  Branches                ?        0           
===============================================
  Hits                    ?     2096           
  Misses                  ?    17384           
  Partials                ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4349232...c7d132f. Read the comment docs.

davidwendt

This looks awesome. I would be curious to know if this improved overall build time as well since hash_functions.cuh is also included in the row_operators.cuh header file.

davidwendt

Just need to remove some unreferenced variables.

cpp/src/hash/md5_hash.cu

vyasr

Nice cleanup. I recommended some further improvements, but be aware that I may not have caught all of the places where those improvements could be made because there seems to be quite a bit of code duplication in this file. I don't know if you want to try and tackle it in this PR or not, but it may be possible to unify a lot of these implementations to a greater degree with some suitably defined device functions. If you think that's out of scope that's fine, just double-check that my suggestions wouldn't also apply in other places.

cpp/src/hash/md5_hash.cu

bdice · 2021-09-15T22:02:16Z

@davidwendt @vyasr Thanks for the great suggestions! I'll follow up on this soon. Many of these suggestions will also help improve #9215.

harrism

I don't have any reason to block this PR beyond other reviewers' comments. Just one question.

cpp/src/hash/md5_hash.cu

bdice · 2021-10-18T23:18:00Z

@vyasr @davidwendt If you want to do a final review of this PR, feel free. I'm aiming to merge by EOD tomorrow to unblock some next steps on SHA work. Thanks!

I ran benchmarks. For a column of int32 values, this code is a bit slower than the previous implementation for large numbers of elements, but I hope that's okay for the moment (18.1 ms in branch-21.12 vs. 20.2 ms in this PR for processing 2^24 elements). I am planning to investigate performance a bit further once I merge this refactored code with the SHA implementation. I suspect the performance might be improved with minor changes that I will apply to both the MD5 and SHA algorithms.

jrhemstad · 2021-10-18T23:30:07Z

@vyasr @davidwendt If you want to do a final review of this PR, feel free. I'm aiming to merge by EOD tomorrow to unblock some next steps on SHA work. Thanks!

I ran benchmarks. For a column of int32 values, this code is a bit slower than the previous implementation for large numbers of elements, but I hope that's okay for the moment (18.1 ms in branch-21.12 vs. 20.2 ms in this PR for processing 2^24 elements). I am planning to investigate performance a bit further once I merge this refactored code with the SHA implementation. I suspect the performance might be improved with minor changes that I will apply to both the MD5 and SHA algorithms.

For performance, I think the lowest hanging fruit is to move the circular_buffer backing storage into shared memory.

bdice · 2021-10-18T23:52:24Z

@jrhemstad I agree shared memory should be a good perf boost -- I'll give that a shot when I refactor with the SHA code.

vyasr

This is a big improvement. Got some further suggestions before merging, but it's almost there. Any performance impact of these changes?

cpp/include/cudf/detail/utilities/hash_functions.cuh

cpp/src/hash/md5_hash.cu

vyasr

This is a big improvement. Got some further suggestions before merging, but it's almost there. Any performance impact of these changes?

cpp/src/hash/md5_hash.cu

bdice · 2021-10-21T18:24:39Z

@vyasr This is ready for a final review.

Any performance impact of these changes?

See #9212 (comment). It's slightly slower but I'll refactor this to use shared memory when refactoring with SHA, which should boost performance.

vyasr

I have a couple small questions but I think this is good to go however you choose to address them.

cpp/src/hash/md5_hash.cu

bdice · 2021-10-21T23:25:15Z

@gpucibot merge

bdice added 5 commits September 9, 2021 18:10

Improve comments and naming.

9d158e8

Remove unused seed from MD5Hash constructor.

5630eed

Expand detail namespace.

f23735d

Remove seed and default constructors.

ed56ef0

Move MD5 implementations from hash_functions.cuh to md5_hash.cu becau…

03bfe6d

…se they are only used in that file.

bdice requested a review from a team as a code owner September 10, 2021 03:10

bdice requested review from harrism and vyasr September 10, 2021 03:10

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Sep 10, 2021

bdice added improvement Improvement / enhancement to an existing function tech debt non-breaking Non-breaking change labels Sep 10, 2021

bdice self-assigned this Sep 10, 2021

davidwendt approved these changes Sep 15, 2021

View reviewed changes

davidwendt requested changes Sep 15, 2021

View reviewed changes

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

vyasr requested changes Sep 15, 2021

View reviewed changes

bdice added a commit to bdice/cudf that referenced this pull request Sep 16, 2021

Revert changes to MD5 (separated into rapidsai#9212).

585bea4

bdice mentioned this pull request Sep 16, 2021

Add SHA-1 and SHA-2 hash functions. #9215

Closed

harrism approved these changes Sep 17, 2021

View reviewed changes

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

Move variable.

ff038cd

jrhemstad approved these changes Oct 18, 2021

View reviewed changes

vyasr requested changes Oct 20, 2021

View reviewed changes

bdice added 6 commits October 19, 2021 18:44

Declare HasherDispatchers inline.

cc2bf4b

Make HasherDispatcher column_device_view const&.

9fd47fa

Parenthesize expression for readability.

f12eaf8

Add const.

2f5c2b6

Rename Key to Element, get_data to get_element_pointer_and_size.

25c0e90

Add constructor to HasherDispatcher.

b6c4c75

davidwendt reviewed Oct 20, 2021

View reviewed changes

cpp/src/hash/md5_hash.cu Show resolved Hide resolved

davidwendt reviewed Oct 21, 2021

View reviewed changes

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

bdice added 3 commits October 21, 2021 07:38

Simplify hash_circular_buffer.

d7ff965

Reformat shift constants.

8cdf423

Add comment.

e7834dd

davidwendt approved these changes Oct 21, 2021

View reviewed changes

bdice added 2 commits October 21, 2021 09:41

Remove unnecessary qualifiers.

92b329b

Add comment about constant sources.

410b30d

bdice requested a review from vyasr October 21, 2021 18:24

vyasr approved these changes Oct 21, 2021

View reviewed changes

cpp/src/hash/md5_hash.cu Outdated Show resolved Hide resolved

cpp/src/hash/md5_hash.cu Show resolved Hide resolved

Clarify flag.

c7d132f

rapids-bot bot merged commit 72694d2 into rapidsai:branch-21.12 Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MD5 implementation. #9212

Refactor MD5 implementation. #9212

bdice commented Sep 10, 2021 •

edited

Loading

bdice commented Sep 14, 2021

codecov bot commented Sep 14, 2021 •

edited

Loading

davidwendt left a comment

davidwendt left a comment

vyasr left a comment

bdice commented Sep 15, 2021

harrism left a comment

bdice commented Oct 18, 2021

jrhemstad commented Oct 18, 2021

bdice commented Oct 18, 2021

vyasr left a comment

vyasr left a comment

bdice commented Oct 21, 2021

vyasr left a comment

bdice commented Oct 21, 2021

Refactor MD5 implementation. #9212

Refactor MD5 implementation. #9212

Conversation

bdice commented Sep 10, 2021 • edited Loading

bdice commented Sep 14, 2021

codecov bot commented Sep 14, 2021 • edited Loading

Codecov Report

davidwendt left a comment

Choose a reason for hiding this comment

davidwendt left a comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

bdice commented Sep 15, 2021

harrism left a comment

Choose a reason for hiding this comment

bdice commented Oct 18, 2021

jrhemstad commented Oct 18, 2021

bdice commented Oct 18, 2021

vyasr left a comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

bdice commented Oct 21, 2021

vyasr left a comment

Choose a reason for hiding this comment

bdice commented Oct 21, 2021

bdice commented Sep 10, 2021 •

edited

Loading

codecov bot commented Sep 14, 2021 •

edited

Loading