Clean up namespaces and improve compression-related headers #17621
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
vuule
added
improvement
|
/ok to test |
cpp/src/io/orc/reader_impl_decode.cu
Outdated
| device_span<device_span<uint8_t const>> inflate_in_view{inflate_in.data(), | ||
| num_compressed_blocks}; | ||
| device_span<device_span<uint8_t>> inflate_out_view{inflate_out.data(), num_compressed_blocks}; |
There was a problem hiding this comment.
unrelated to header changes, just noticed that the spans are created for no reason here.
| @@ -44,7 +45,11 @@ namespace io { | |||
| namespace orc { | |||
| namespace gpu { | |||
|
|
|||
| namespace nvcomp = cudf::io::detail::nvcomp; | |||
There was a problem hiding this comment.
I don't love that we have an internal namespace with the same name as nvcomp's own C++ namespace. This feels like it's asking for a resolution error to occur eventually. At the same time, I don't feel strongly enough to request a change.
There was a problem hiding this comment.
I've thought about this (briefly :D ), and went ahead because I've never seen a namespace in nvcomp; the header files we include are extern "C" { in the global namespace. Also, no file outside of the adapter should include anything from nvcomp.
FWIW, these files should stop depending on the nvcomp adapter (and thus the namespace) once I implement the high-level API that uses nvcomp transparently.
There was a problem hiding this comment.
all that said, I should have used a different name :D
There was a problem hiding this comment.
I checked that nvcomp.hpp has namespace nvcomp, but I did not look at whether it has extern C. Let’s not worry unless it presents problems.
| if (log_cyclecount) { | ||
| results[strm_id].reserved = clock() - s->tstart; | ||
| } else { | ||
| results[strm_id].reserved = 0; | ||
| } |
There was a problem hiding this comment.
Has the cycle count logic been relocated elsewhere, or is it simply being removed?
There was a problem hiding this comment.
Good question, I forgot to comment on this.
It's removed: It's unused and keeping it just adds noise to the future API changes.
Co-authored-by: Bradley Dice <[email protected]>
|
/merge |
Description
Moved compression-related stuff that was under
cudf::iotocudf::io::detail.Moved the nvcomp adapter from
cudf::io::nvcomptocudf::io::detail::nvcomp.Extract common compression constants to appropriate headers, and updated the files to include what they use.
Changes are made in preparation for adding higher-level compression API that abstracts nvcomp use and simplifies caller code.
Checklist