[PERF]: Improve the performance of plc.io.parquet.read_parquet with prefetched parquet file metadata. - #23558
[PERF]: Improve the performance of plc.io.parquet.read_parquet with prefetched parquet file metadata.#23558TomAugspurger wants to merge 21 commits into
plc.io.parquet.read_parquet with prefetched parquet file metadata.#23558Conversation
TomAugspurger
added
Performance
|
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. |
github-actions
Bot
added
libcudf
|
/ok to test 39cc045 |
|
This change did help with the metadata prefetching. But I still see GIL contention in Scan nodes: 
|
|
/ok to test b4d6a87 |
This changes the ownership model of FileMetadata to own the footers via unique_ptr and move them (under nogil) in the pylibcudf wrapper. Additionally, we avoid an unnecessary copy in get_parquet_metadatas().
b4d6a87 to
1ac2985
Compare
|
/ok to test 1ac2985 |
|
/ok to test af347e8 |
When page indexes are present, cloning the parquet footer FileMetaData object can be expensive. This slows down `read_parquet` when the user provides a prefetched metadata object. This PR adds a new `read_page_indexes` parameter to `read_parquet_footers` that controls whether page indexes are materialized. The default is `True` matching the existing behavior.
3d8de45 to
b3775f5
Compare
|
/ok to test f5525b5 |
TomAugspurger
changed the title
read_parquet with prefetched parquet file metadata.
TomAugspurger
changed the title
read_parquet with prefetched parquet file metadata.plc.io.parquet.read_parquet with prefetched parquet file metadata.
|
/ok to test 0053fb4 |
|
These might overlap conceptually with #23546. |
| def version(self): | ||
| """Get the file format version.""" | ||
| return self.c_obj.version | ||
| return dereference(self.c_obj).version |
There was a problem hiding this comment.
Should we add a __cinit__ that default-constructs self.c_obj? Otherwise it's null in any instance not produced by from_libcudf (e.g., via FileMetaData.__new__(FileMetaData)), and this will segfault (as will any other dereference(), including the one in hybrid_scan.pyx).
There was a problem hiding this comment.
Because we have __init__ raising, I think it's not easily possible to make a FileMetadata except via from_libcudf. This is a typical pattern in the pylibcudf bindings IIRC.
| # Set the engine-dependent default, but don't override any user-provided values | ||
| # in-memory or via the environment. | ||
| if "prefetch_file_metadata" not in user_parquet_options and ( | ||
| os.environ.get(f"{ParquetOptions._env_prefix}__PREFETCH_FILE_METADATA") |
There was a problem hiding this comment.
Do we need to have an equivalent fix-up in the else branch? Otherwise the config passes through the incoming non-dict options object, and test_parquet_options_object_passthrough only tests with prefetch_file_metadata=False…
| def version(self): | ||
| """Get the file format version.""" | ||
| return self.c_obj.version | ||
| return dereference(self.c_obj).version |
There was a problem hiding this comment.
Because we have __init__ raising, I think it's not easily possible to make a FileMetadata except via from_libcudf. This is a typical pattern in the pylibcudf bindings IIRC.
|
/ok to test 93bccd7 |
|
/ok to test 8b7563a |
6 participants
Description
We've observed a performance penalty from enabling metadata prefetching in cudf-polars for data in local storage. We'd like to enable prefetching by default, since it's crucial for good performance with high-latency remote storage systems (S3). Also, you'd naively expect prefetching to help locally since you'd do less work (you avoid re-parsing the footer when reading different row groups out of the same file multiple times).
Over simplifying things, prefetching slowed things down because it's (surprisingly?) expensive to copy a
FileMetaDataobject. This pain was compounded in cudf-polars because we run concurrentread_parquets in a thread pool, and the GIL was held for these expensive copies.(Note: why are we copying in the first place? IIUC, it's because the parquet reader currently mutates something on the object in
read_parquet, so it needs an ownedFileMetaData.)This PR moves FileMetaData footers into Python without holding the GIL. This lets concurrent prefetches happen more... concurrently. See https://github.com/user-attachments/assets/c0f34117-8483-413a-a3de-e101a0093c12 for an nsys profile screenshot showing GIL contention.
After this fix, prefetching is still a clear win on S3 but there's still a ~16% penalty on NVMe. Rather than holding off entirely, this PR enables prefetching by default only for remote URIs (s3://, gs://, etc.). And queries that mix remote and local reads will only prefetch for the remote reads if metedata prefetching is unset (default).
Benchmarks (SF100, all 22 TPC-H queries, 256 kvikio threads):