Add BGZIP multibyte_split benchmark #11723

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.

cpp/benchmarks/io/text/multibyte_split.cpp

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?

cpp/src/io/text/bgzip_utils.hpp

vuule

Looks pretty good. Some minor comments/questions.

cpp/tests/io/text/data_chunk_source_test.cpp

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!

cpp/src/io/text/bgzip_utils.cpp

vuule

🔥

cpp/benchmarks/io/text/multibyte_split.cpp

* 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.

cpp/include/cudf/io/text/detail/bgzip_utils.hpp

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 harrism and PointKernel 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

cpp/benchmarks/io/text/multibyte_split.cpp Outdated Show resolved Hide resolved

ttnghia reviewed Sep 27, 2022

View reviewed changes

cpp/benchmarks/io/text/multibyte_split.cpp Outdated Show resolved Hide resolved

ttnghia reviewed Sep 27, 2022

View reviewed changes

cpp/benchmarks/io/text/multibyte_split.cpp Outdated Show resolved Hide resolved

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 added 2 commits September 28, 2022 08:47

bgzip refactoring and benchmark

d8feee1

improve naming

848fc5e

vuule reviewed Sep 29, 2022

View reviewed changes

cpp/src/io/text/bgzip_utils.hpp Outdated Show resolved Hide resolved

vuule reviewed Sep 30, 2022

View reviewed changes

upsj 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

cpp/src/io/text/bgzip_utils.cpp Outdated Show resolved Hide resolved

cpp/src/io/text/bgzip_utils.cpp Outdated Show resolved Hide resolved

cpp/src/io/text/bgzip_utils.cpp Outdated Show resolved Hide resolved

cpp/src/io/text/bgzip_utils.cpp Outdated Show resolved Hide resolved

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

cpp/benchmarks/io/text/multibyte_split.cpp Outdated Show resolved Hide resolved

cpp/benchmarks/io/text/multibyte_split.cpp Outdated Show resolved Hide resolved

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

cpp/include/cudf/io/text/detail/bgzip_utils.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/io/text/detail/bgzip_utils.hpp Outdated Show resolved Hide resolved

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 rapidsai:branch-22.12 Oct 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BGZIP multibyte_split benchmark #11723

Add BGZIP multibyte_split benchmark #11723

upsj commented Sep 20, 2022 •

edited

Loading

PointKernel commented Sep 20, 2022

upsj commented Sep 21, 2022

robertmaynard left a comment

PointKernel commented Sep 21, 2022

ttnghia left a comment

vuule commented Sep 27, 2022

vuule left a comment

upsj commented Oct 3, 2022

codecov bot commented Oct 3, 2022 •

edited

Loading

PointKernel commented Oct 3, 2022 •

edited

Loading

upsj commented Oct 4, 2022

PointKernel left a comment

vuule left a comment

bdice left a comment

upsj commented Oct 10, 2022

Add BGZIP multibyte_split benchmark #11723

Add BGZIP multibyte_split benchmark #11723

Conversation

upsj commented Sep 20, 2022 • edited Loading

Description

[0] Tesla T4

Checklist

PointKernel commented Sep 20, 2022

upsj commented Sep 21, 2022

robertmaynard left a comment

Choose a reason for hiding this comment

PointKernel commented Sep 21, 2022

ttnghia left a comment

Choose a reason for hiding this comment

vuule commented Sep 27, 2022

vuule left a comment

Choose a reason for hiding this comment

upsj commented Oct 3, 2022

codecov bot commented Oct 3, 2022 • edited Loading

Codecov Report

PointKernel commented Oct 3, 2022 • edited Loading

upsj commented Oct 4, 2022

PointKernel left a comment

Choose a reason for hiding this comment

vuule left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

upsj commented Oct 10, 2022

upsj commented Sep 20, 2022 •

edited

Loading

codecov bot commented Oct 3, 2022 •

edited

Loading

PointKernel commented Oct 3, 2022 •

edited

Loading