Add an I/O observation facility - #1033
Conversation
d42da15 to
0dece5b
Compare
`nbytes()` caches the file size and `write()` invalidates it, but two `pwrite()` paths never go through `write()`: the host path and the sub-threshold device shortcut. So `nbytes()` could report a stale size after a write. Both now invalidate the cache once the write has completed, so a `nbytes()` call racing with an in-flight `pwrite()` cannot leave a stale size cached either. Unrelated to the rest of this branch.
e1bf9bf to
7360ca8
Compare
| if (is_read_out_of_bounds(file_offset, size, _nbytes)) { | ||
| std::stringstream ss; | ||
| ss << "cannot read " << file_offset << "+" << size << " bytes into a " << _nbytes | ||
| << " bytes file (" << _endpoint->str() << ")"; | ||
| KVIKIO_FAIL(ss.str(), std::invalid_argument); | ||
| } |
There was a problem hiding this comment.
Not removed, just moved to the top of read, before the buffer is classified.
| if (is_read_out_of_bounds(file_offset, size, _nbytes)) { | ||
| std::stringstream ss; | ||
| ss << "cannot read " << file_offset << "+" << size << " bytes into a " << _nbytes | ||
| << " bytes file (" << _endpoint->str() << ")"; | ||
| KVIKIO_FAIL(ss.str(), std::invalid_argument); | ||
| } |
There was a problem hiding this comment.
Not removed, just moved to the top of read, before the buffer is classified.
7360ca8 to
0846f46
Compare
0846f46 to
9a4fe49
Compare
wence-
left a comment
There was a problem hiding this comment.
Broadly looks good, I think.
| ObservationKind kind{ObservationKind::Logical}; | ||
|
|
||
| /// When the operation started. | ||
| TimePoint start{}; | ||
| /// When the operation finished. | ||
| TimePoint end{}; | ||
| /// Byte offset into the file or remote object. | ||
| std::size_t offset{}; | ||
| /// Number of bytes requested. | ||
| std::size_t size{}; | ||
| /// Number of bytes actually transferred. Differs from `size` on a short read, and is zero for an | ||
| /// operation that failed. | ||
| std::size_t bytes_transferred{}; | ||
|
|
||
| /// The backend that carried out the operation. | ||
| IoBackend backend{IoBackend::Posix}; | ||
| /// The direction of the operation. | ||
| TransferDirection direction{TransferDirection::Read}; | ||
| /// The kind of memory the caller's buffer lives in. | ||
| MemoryKind memory_kind{MemoryKind::Host}; | ||
| /// False if the operation failed. | ||
| bool ok{true}; | ||
| /// Identifies this operation, uniquely within the process. | ||
| std::uint64_t id{}; | ||
|
|
||
| /// HTTP method, e.g. `"GET"`. Null for local I/O. | ||
| char const* http_method{nullptr}; |
There was a problem hiding this comment.
nit: Check the layout of this struct for potentially more padding than necessary
There was a problem hiding this comment.
Good catch, moved kind down to join the other small enums and id up with the other 8-byte fields. Now
64 bytes with a single 3-byte tail hole.
| KVIKIO_NVTX_FUNC_RANGE(size); | ||
| if (get_compat_mode_manager().is_compat_mode_preferred()) { | ||
| return detail::posix_device_read( | ||
| detail::expect_not_in_monitor(); |
There was a problem hiding this comment.
question: Should we make this a no-op unless in debug mode?
There was a problem hiding this comment.
I would rather keep it in release. It is one thread-local bool load per user-facing call, and the whole disabled observation path measured at about 3 ns.
What it buys is a clear error instead of a hang.
| // A monitor registered after this operation began never saw its start and ignores the finish, | ||
| // which is why no flag is latched here. | ||
| notify_finished(_observation); | ||
| } |
There was a problem hiding this comment.
I don't understand this comment
There was a problem hiding this comment.
Fair, it was too compressed. It was trying to say why the recorder does not track which monitors it notified. Rewritten as:
// Delivered to whoever is registered now, which need not be who was registered when the
// operation began. A monitor that missed the start ignores this, as `Monitor::on_finish()`
// documents, so the recorder does not have to remember which monitors it told.| if (o.size == 0 || o.id == 0 || o.start == kvikio::TimePoint{}) { ++_malformed; } | ||
| if (o.end != kvikio::TimePoint{} || o.bytes_transferred != 0) { ++_malformed; } |
There was a problem hiding this comment.
I don't understand this checking of the start against the timepoint. These could match/mismatch validly, no?
There was a problem hiding this comment.
You are right TimePoint{} is the clock epoch, so it is a stand-in for "unset" that a real timestamp could in principle collide with.
Replaced with something that has content: the map now holds the whole Observation, and on_finish() checks the completion carries the same size and the same start as the record seen at submission.
This PR introduces a hook into monitoring KvikIO operations, with the goal of building statistics, Quent timelines, and whatever else wants to know what the I/O layer is doing. This PR is the base. A follow-up introduces a concrete
Monitorthat makes statistics easy to get.The hook reports whole KvikIO operations (the logical level) so a
pread()is a single observation however many reads the thread pool issued underneath. Physical operations can be added along the same path later, which could be the basis of #1016.Each call produces a
kvikio::Observation: its span, the offset and size etc. To receive them, derive fromkvikio::Monitorand register it. A monitor is told when an operation starts as well as when it finishes.Overhead
Measured on my local workstation:
pread(), and nothing detectable at a megabyte.Confirmed against a real workload: cudf-polars PDS-H query 1 at scale 10, with and without a monitor attached, showed no difference outside noise.
What is not observed
The cuFile asynchronous API on a GDS system, and the batch API, complete without KvikIO seeing it, so they emit nothing. Handling those needs a stream-completion callback, which is future work.
Follow-up: statistics
The next PR adds
kvikio::statistics::SummaryMonitor, which is aMonitorand nothing more:Those are real numbers, from a cudf-polars run, and they show a very useful
busy bandwidth. 8.95 GB/s is the rate while KvikIO actually had work in flight, where dividing the same bytes by the wall clock would have said 1.75 GB/s and described the query rather than the storage.A
TimelineMonitor, for when things happened rather than how much, is planned after that.