ROS3 VFD page cache feature#6478
Conversation
Implements a minimal I/O "page" cache for files without paged allocation enabled to better optimize I/O and reduce requests to S3 I/O is cached in fixed-size pages (16MiB pages by default) and are kept in a simple LRU cache that evicts the oldest used page when a new page needs to be cached Reading and caching of the initial bytes of a file has been delayed from file open to the first read for a file instead API functions have been added for setting/getting the I/O page caching parameters to be used
hyoklee
left a comment
There was a problem hiding this comment.
I don't see any significant performance improvement backed by real data and numbers in release_docs.
"Significant performance improvement" is not the main focus of this PR, reducing the number of requests to S3 is the main focus. Depending on the file in question and the I/O access pattern, there may be no actual benefit. Data is still being gathered on the improvements for files that do not use paged file space allocation, but listing any concrete numbers in CHANGELOG.md will not offer much insight, as any improvements will depend on I/O access pattern. |
| page_buf_size = ROS3_DEF_PAGE_BUF_SIZE; | ||
| } | ||
|
|
||
| file->page_cache.max_num_pages = (page_buf_size / file->page_cache.page_size); |
There was a problem hiding this comment.
If H5Pset_page_buffer_size() is used to set the page buffer below 16 MiB and H5Pset_fapl_ros3_paging() is not used, this computation will yield zero.
It seems like this can cause a crash during a later read. In the main caching loop, make_space's HASH_COUNT < max_num_pages check will resolve false, so it would attempt an eviction on an empty LRU list. With the default lock_super_page = true, the superblock page isn't in the LRU, so LRU_tail would be NULL and ROS3_PAGE_CACHE_LRU_REMOVE would try to dereference a null page.
Just clamping max_num_pages to a minimum of 1 here should resolve this.
There was a problem hiding this comment.
This is true; the page buffer size should act as the final say of how many bytes can be allocated, so the page size should be rounded down to the page buffer size in that case, similar to what H5Pset_fapl_ros3_paging() does.
There was a problem hiding this comment.
I'm talking about the value for max_num_pages, not the size of the pages themselves. It seems like storing zero for the max page number could lead to the crash I described later on.
There was a problem hiding this comment.
Yes, when the chosen page buffer size is smaller than the default page size, the page size should be rounded down to the page buffer size, resulting in 1 for max_num_pages. Just rounding max_num_pages up to 1 without adjusting the page size would result in a 16MiB (by default) page being allocated even though the "caller" asked for an upper limit on the page buffer size that's smaller than 16MiB.
|
|
||
| done: | ||
| FUNC_LEAVE_NOAPI(ret_value) | ||
| } /* end H5FD__ros3_determine_io_pages() */ |
There was a problem hiding this comment.
Ending comment here has the wrong function name
88c8602 to
c857f9a
Compare
Did you finish gathering data? |
|
@jhendersonHDF , please update |
Implements a minimal I/O "page" cache for files without paged allocation enabled to better optimize I/O and reduce requests to S3
I/O is cached in fixed-size pages (16MiB pages by default) and are kept in a simple LRU cache that evicts the oldest used page when a new page needs to be cached
Reading and caching of the initial bytes of a file has been delayed from file open to the first read for a file instead
API functions have been added for setting/getting the I/O page caching parameters to be used