[RFC] Refactor block cache tracing APIs

[anand1976]

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.

[zhichao-cao]

Do we need "WriteFooter"?

[anand1976]

Yes, we could call it on EndTrace() to keep it symmetrical.

[mrambacher]

Why not just use bool?

[anand1976]

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};.

[mrambacher]

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.

[mrambacher]

At the very least, the class should have a "virtual const char *Name() const = 0" to allow alternate implementations later.

[anand1976]

I plan to make it a Customizable class.

[mrambacher]

Can you add a comment stating what this method does?

[mrambacher]

Shouldn't this be an ASSERT_OK?

[anand1976]

Thanks @zhichao-cao and @mrambacher for the review!

[anand1976]

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};.

[anand1976]

I plan to make it a Customizable class.

[anand1976]

Yes, we could call it on EndTrace() to keep it symmetrical.

[mdcallag]

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.

[riversand963]

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"?

[anand1976]

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.

@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

[zhichao-cao]

Should we indicate that the class/interfaces/structs in this file is experimental and may be changed in the future?

[zhichao-cao]

So, this change will not continue?