Add BGZIP multibyte_split benchmark by upsj · Pull Request #11723 · NVIDIA/cudf

upsj · 2022-09-20T19:34:30Z

Description

This refactors #11652 to extract the BGZIP IO and adds another source_type to the multibyte_split benchmark, creating a compressed file using zlib.

A quick benchmark shows performance results around 2.5x slower than reading from a device buffer at around 1:5 compression ratio

[0] Tesla T4

source_type	delim_size	delim_percent	size_approx	byte_range_percent	Time	Peak Memory Usage	Encoded file size
bgzip	1	1	2^30 = 1073741824	100	507.479 ms	4.022 GiB	1006.638 MiB
file	1	1	2^30 = 1073741824	100	339.860 ms	3.947 GiB	1006.638 MiB
device	1	1	2^30 = 1073741824	100	201.556 ms	3.947 GiB	1006.638 MiB

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

PointKernel · 2022-09-20T23:15:28Z

@upsj Is this PR depending on #11652?

upsj · 2022-09-21T05:53:45Z

@PointKernel yes, should we wait with the review until #11652 is merged? Many of the suggestions also apply to the refactored code.

robertmaynard

CMake changes LGTM

PointKernel · 2022-09-21T17:05:20Z

should we wait with the review until #11652 is merged?

Yes please, this will avoid unnecessary back-and-forth effort.

ttnghia

This seems to be larger than just a benchmark.

vuule · 2022-09-27T19:25:00Z

This seems to be larger than just a benchmark.

I think 22.10 needs to be merged so #11652 changes are excluded. @upsj do you want to keep this targeted for 22.10?

upsj · 2022-09-28T09:18:33Z

@vuule @ttnghia apologies for the confusion, the PR was still not up-to-date. I am used to a rebase-then-merge workflow rather than a merge-then-squash, so I hope me rebasing the branch doesn't mess up your review workflow. It should be ready now. It's not important to get this into 22.10

upsj · 2022-09-28T21:39:35Z

rerun tests

upsj · 2022-09-29T18:09:09Z

rerun tests

vuule

Looks pretty good. Some minor comments/questions.

upsj · 2022-10-03T20:24:21Z

rerun tests

codecov · 2022-10-03T22:40:09Z

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.12@a270ae6). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@               Coverage Diff               @@
##             branch-22.12   #11723   +/-   ##
===============================================
  Coverage                ?   87.48%           
===============================================
  Files                   ?      133           
  Lines                   ?    21866           
  Branches                ?        0           
===============================================
  Hits                    ?    19130           
  Misses                  ?     2736           
  Partials                ?        0

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

PointKernel · 2022-10-03T23:03:07Z

source_type
bgzip
file
device

@upsj How did you print out (display) the source type without using NVBENCH_DECLARE_ENUM_TYPE_STRINGS?

upsj · 2022-10-04T05:39:59Z

@PointKernel I didn't 😉 I edited the output manually for clarity, but thanks for reminding me of the macro, I'll use it instead

PointKernel

Some nitpick. Looks great!

vuule

🔥

* move header to include * more constexpr * rename ostreams to output_stream * fewer magic numbers

bdice

A couple small comments that you can handle as you wish, otherwise LGTM.

upsj · 2022-10-10T07:56:07Z

@gpucibot merge

upsj added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 20, 2022

upsj added this to the Genomics read_text support milestone Sep 20, 2022

upsj self-assigned this Sep 20, 2022

upsj requested review from a team as code owners September 20, 2022 19:34

upsj requested review from PointKernel and harrism September 20, 2022 19:34

github-actions Bot added the CMake CMake build issue label Sep 20, 2022

upsj requested review from vuule and removed request for harrism September 21, 2022 06:00

robertmaynard approved these changes Sep 21, 2022

View reviewed changes

upsj removed the 3 - Ready for Review Ready for review by team label Sep 21, 2022

upsj mentioned this pull request Sep 26, 2022

Add BGZIP data_chunk_reader #11652

Merged

7 tasks

ttnghia reviewed Sep 27, 2022

View reviewed changes

Comment thread cpp/benchmarks/io/text/multibyte_split.cpp Outdated

ttnghia reviewed Sep 27, 2022

View reviewed changes

Comment thread cpp/benchmarks/io/text/multibyte_split.cpp Outdated

ttnghia reviewed Sep 27, 2022

View reviewed changes

Comment thread cpp/benchmarks/io/text/multibyte_split.cpp Outdated

vuule added the 0 - Waiting on Author Waiting for author to respond to review label Sep 27, 2022

ttnghia reviewed Sep 27, 2022

View reviewed changes

upsj-nv added 2 commits September 28, 2022 08:47

bgzip refactoring and benchmark

d8feee1

improve naming

848fc5e

upsj force-pushed the feature/bgzip_benchmark branch from 19f52a8 to 848fc5e Compare September 28, 2022 09:14

upsj added 3 - Ready for Review Ready for review by team and removed 0 - Waiting on Author Waiting for author to respond to review labels Sep 28, 2022

upsj added the cuIO cuIO issue label Sep 29, 2022

vuule reviewed Sep 29, 2022

View reviewed changes

Comment thread cpp/src/io/text/bgzip_utils.hpp

vuule reviewed Sep 30, 2022

View reviewed changes

Comment thread cpp/tests/io/text/data_chunk_source_test.cpp Outdated

Comment thread cpp/tests/io/text/data_chunk_source_test.cpp Outdated

Comment thread cpp/tests/io/text/data_chunk_source_test.cpp Outdated

Comment thread cpp/tests/io/text/data_chunk_source_test.cpp

upsj-nv added 3 commits October 4, 2022 09:17

move code to source

b707e1f

improve test readability

5b8d356

use enum_type for source_type

d176cee

PointKernel reviewed Oct 4, 2022

View reviewed changes

Comment thread cpp/src/io/text/bgzip_utils.cpp Outdated

Comment thread cpp/src/io/text/bgzip_utils.cpp Outdated

Comment thread cpp/src/io/text/bgzip_utils.cpp Outdated

Comment thread cpp/src/io/text/bgzip_utils.cpp Outdated

vuule changed the base branch from branch-22.12 to branch-22.10 October 4, 2022 16:06

vuule changed the base branch from branch-22.10 to branch-22.12 October 4, 2022 16:06

vuule approved these changes Oct 4, 2022

View reviewed changes

bdice reviewed Oct 5, 2022

View reviewed changes

Comment thread cpp/benchmarks/io/text/multibyte_split.cpp Outdated

Comment thread cpp/benchmarks/io/text/multibyte_split.cpp Outdated

review updates

8632e27

* move header to include * more constexpr * rename ostreams to output_stream * fewer magic numbers

bdice approved these changes Oct 7, 2022

View reviewed changes

Comment thread cpp/include/cudf/io/text/detail/bgzip_utils.hpp Outdated

Comment thread cpp/include/cudf/io/text/detail/bgzip_utils.hpp Outdated

upsj added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Oct 8, 2022

improve header placement, rename parameters

f6b1d26

upsj requested a review from a team as a code owner October 9, 2022 08:30

github-actions Bot added the conda label Oct 9, 2022

upsj removed the 5 - Ready to Merge Testing and reviews complete, ready to merge label Oct 9, 2022

jjacobelli approved these changes Oct 10, 2022

View reviewed changes

upsj added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Oct 10, 2022

rapids-bot Bot merged commit 4eb9c6c into NVIDIA:branch-22.12 Oct 10, 2022

Uh oh!

Conversation

upsj commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

[0] Tesla T4

Checklist

Uh oh!

PointKernel commented Sep 20, 2022

Uh oh!

upsj commented Sep 21, 2022

Uh oh!

robertmaynard left a comment

Choose a reason for hiding this comment

Uh oh!

PointKernel commented Sep 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ttnghia left a comment

Choose a reason for hiding this comment

Uh oh!

vuule commented Sep 27, 2022

Uh oh!

upsj commented Sep 28, 2022

Uh oh!

upsj commented Sep 28, 2022

Uh oh!

upsj commented Sep 29, 2022

Uh oh!

Uh oh!

vuule left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

upsj commented Oct 3, 2022

Uh oh!

codecov Bot commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PointKernel commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsj commented Oct 4, 2022

Uh oh!

PointKernel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vuule left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bdice left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

upsj commented Oct 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

upsj commented Sep 20, 2022 •

edited

Loading

codecov Bot commented Oct 3, 2022 •

edited

Loading

PointKernel commented Oct 3, 2022 •

edited

Loading