Skip to content

Optimizing multi-source byte range reading in JSON reader - #15396

Merged
rapids-bot[bot] merged 48 commits into
NVIDIA:branch-24.06from
shrshi:byte-range-improvement
Apr 30, 2024
Merged

Optimizing multi-source byte range reading in JSON reader#15396
rapids-bot[bot] merged 48 commits into
NVIDIA:branch-24.06from
shrshi:byte-range-improvement

Conversation

@shrshi

@shrshi shrshi commented Mar 26, 2024

Copy link
Copy Markdown
Contributor

Description

This piece of work seeks to achieve two goals - (i) reducing repeated reading of byte range chunks in the JSON reader, and (ii) enabling multi-source byte range reading for chunks spanning sources.

  • We expand on the idea outlined in Reduce IO when byte_range option is used in read_json #15185 to reduce the repeated reading of follow-on chunks while searching for the end of the last row in the requested chunk. After the requested chunk, the following chunks are divided into subchunks, and read until the delimiter character is reached.
  • We estimate the buffer size needed for the entire byte range, and compute offsets per source into the buffer.

Visualization of the performance improvement with this optimization

Checklist

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

@github-actions github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Mar 26, 2024
@shrshi shrshi added feature request New feature or request Performance Performance related issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed feature request New feature or request labels Mar 26, 2024
@shrshi
shrshi marked this pull request as ready for review March 27, 2024 16:02
@shrshi
shrshi requested a review from a team as a code owner March 27, 2024 16:03

@vuule vuule left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestions with a side of scope creep :D

Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
@copy-pr-bot

copy-pr-bot Bot commented Apr 5, 2024

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@shrshi

shrshi commented Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi
shrshi marked this pull request as draft April 5, 2024 10:21
@shrshi

shrshi commented Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi

shrshi commented Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi

shrshi commented Apr 5, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi

shrshi commented Apr 23, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi

shrshi commented Apr 24, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi

shrshi commented Apr 25, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

Comment thread cpp/src/io/json/json_normalization.cu Outdated
Comment on lines +322 to +323
datasource::owning_buffer<rmm::device_uvector<char>> outdata(std::move(outbuf));
std::swap(indata, outdata);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it's significant, but I didn't grasp it.
What's the advantage of swapping in place over, say, assigning to indata?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reasoning here was that after the RMM buffer in indata datasource has been normalized, we can discard the buffer. Rather than copying outdata to indata with the implicitly generated copy assignment operator in owning_buffer, I thought swapping it would be faster.

Comment thread cpp/src/io/json/json_normalization.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated

@mythrocks mythrocks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nitpicks, and a clarifying question.

@hyperbolic2346 hyperbolic2346 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits and questions

Comment thread cpp/src/io/json/json_normalization.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
// of subchunks.
size_t buffer_size =
reader_compression != compression_type::NONE
? total_source_size * compression_ratio + 4096

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding 4096? Is that for headers?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 4096 is for the headers. I've used the uncompressed buffer size estimate from https://github.com/rapidsai/cudf/blob/064dd7b02166cc67e882b708d66621bc3fafd70b/cpp/src/io/comp/uncomp.cpp#L361

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have this guess defined instead of a hard-coded value. I'm ok either way though.

Comment thread cpp/src/io/json/read_json.cu Outdated
@shrshi

shrshi commented Apr 29, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi

shrshi commented Apr 29, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi
shrshi requested a review from hyperbolic2346 April 29, 2024 23:16

@hyperbolic2346 hyperbolic2346 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits from me this time.

Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
Comment thread cpp/src/io/json/read_json.cu Outdated
@shrshi

shrshi commented Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

/ok to test

@shrshi

shrshi commented Apr 30, 2024

Copy link
Copy Markdown
Contributor Author

/merge

@rapids-bot
rapids-bot Bot merged commit f3206ea into NVIDIA:branch-24.06 Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Performance Performance related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants