[RFC] Refactor block cache tracing APIs #9326
Conversation
|
|
||
| // Write a trace header at the beginning, typically on initiating a trace, | ||
| // with some metadata like a magic number and RocksDB version. | ||
| virtual Status WriteHeader() = 0; |
There was a problem hiding this comment.
Do we need "WriteFooter"?
There was a problem hiding this comment.
Yes, we could call it on EndTrace() to keep it symmetrical.
| #include "rocksdb/trace_record.h" | ||
|
|
||
| namespace ROCKSDB_NAMESPACE { | ||
| enum Boolean : char { kTrue = 1, kFalse = 0 }; |
There was a problem hiding this comment.
Good question. The original definition of BlockCacheTraceRecord in block_cache_tracer.h used it and I just moved the definitions. There is a case for using an enum instead of bool if the constants have descriptive names, as it improves readability. But in this case, the constants are just kTrue and kFalse, which seems rather pointless. It might make more sense if we use something like enum CacheLookupResult : char { kCacheHit = 1, kCacheMiss = 0};.
| std::unique_ptr<BlockCacheTraceWriter> NewBlockCacheTraceWriter( | ||
| SystemClock* clock, const BlockCacheTraceWriterOptions& trace_options, | ||
| std::unique_ptr<TraceWriter>&& trace_writer); | ||
|
|
There was a problem hiding this comment.
Why not make the BlockCacheTraceWriter a Customizable class and use that infrastructure? That will make it simpler if you ever intend to add alternative implementations.
| // BlockCacheTraceWriter is an abstract class that captures all RocksDB block | ||
| // cache accesses. Every RocksDB operation is passed to WriteBlockAccess() | ||
| // with a BlockCacheTraceRecord. | ||
| class BlockCacheTraceWriter { |
There was a problem hiding this comment.
At the very least, the class should have a "virtual const char *Name() const = 0" to allow alternate implementations later.
There was a problem hiding this comment.
I plan to make it a Customizable class.
| public: | ||
| virtual ~BlockCacheTraceWriter() {} | ||
|
|
||
| // Pass Slice references to avoid copy. |
There was a problem hiding this comment.
Can you add a comment stating what this method does?
| assert(c->block_cache_tracer_ | ||
| .StartTrace(env_->GetSystemClock().get(), trace_opt, | ||
| std::move(trace_writer)) | ||
| .StartTrace(trace_opt, std::move(block_cache_trace_writer)) | ||
| .ok()); |
There was a problem hiding this comment.
Shouldn't this be an ASSERT_OK?
| #include "rocksdb/trace_record.h" | ||
|
|
||
| namespace ROCKSDB_NAMESPACE { | ||
| enum Boolean : char { kTrue = 1, kFalse = 0 }; |
There was a problem hiding this comment.
Good question. The original definition of BlockCacheTraceRecord in block_cache_tracer.h used it and I just moved the definitions. There is a case for using an enum instead of bool if the constants have descriptive names, as it improves readability. But in this case, the constants are just kTrue and kFalse, which seems rather pointless. It might make more sense if we use something like enum CacheLookupResult : char { kCacheHit = 1, kCacheMiss = 0};.
| // BlockCacheTraceWriter is an abstract class that captures all RocksDB block | ||
| // cache accesses. Every RocksDB operation is passed to WriteBlockAccess() | ||
| // with a BlockCacheTraceRecord. | ||
| class BlockCacheTraceWriter { |
There was a problem hiding this comment.
I plan to make it a Customizable class.
|
|
||
| // Write a trace header at the beginning, typically on initiating a trace, | ||
| // with some metadata like a magic number and RocksDB version. | ||
| virtual Status WriteHeader() = 0; |
There was a problem hiding this comment.
Yes, we could call it on EndTrace() to keep it symmetrical.
|
Block cache tracing caused the largest regression early in the 6.x cycle, it was eventually fixed (or mostly fixed). Do you have results to confirm this doesn't add CPU overhead when disabled? db_bench --benchmarks=fillseq,readrandom should be sufficient. |
Thanks @mdcallag for the input. Just to catch up, are you referring to a8975b6 as the "eventual fix"? |
@mdcallag I haven't run any benchmarks yet, but will do and ensure there's no regression. This was a draft in order to get some early feedback |
anand1976
force-pushed
the
tracing_refactor
branch
from
64ba0e9
to
89f6722
Compare
|
|
||
| namespace ROCKSDB_NAMESPACE { | ||
| // A record for block cache lookups/inserts. This is passed by the table | ||
| // reader to the BlockCacheTraceWriter for every block cache op. |
There was a problem hiding this comment.
Should we indicate that the class/interfaces/structs in this file is experimental and may be changed in the future?
|
So, this change will not continue? |
Refactor the classes, APIs and data structures for block cache tracing to allow a user provided trace writer to be used. Currently, only a TraceWriter is supported, with a default built-in implementation of FileTraceWriter. The TraceWriter, however, takes a flat trace record and is thus only suitable for file tracing. This PR introduces an abstract BlockCacheTraceWriter class that takes a structured BlockCacheTraceRecord. The BlockCacheTraceWriter implementation can then format and log the record in whatever way it sees fit. The default BlockCacheTraceWriterImpl does file tracing using a user provided TraceWriter.
This same philosophy can be applied to KV and IO tracing as well.