Closed reecewayt closed 2 weeks ago
@reecewayt I think this is generally looking good, but I'm wondering if you can rebase your changes onto main
in such a way that there aren't a bunch of merge commits duplicating the changes already in main
. It's kind of difficult to review at the moment, because if I view all the changes at once, there's a ton of stuff unrelated to the logger work. And if I try to review just the commits I think are for the logger work, it's incremental, so I can't see the end product all at once. (Plus, we don't really want those duplicate commits in main
anyways...)
It's possible it would be easiest at this point to create a new branch off of main
and cherry-pick your logger commits. Or you could just add the changes so that each logical chunk of work is in a single commit, e.g. one commit with the completed logger, a separate one for all the testing stuff.
Another initial thought I had is that it could be nice to have the statistics separate from the logger. That would mean we could change our output method or use a different logger, but still have the stats. I think the interface looks good (from a quick review), but it seems to make more sense to me to call something like stats.record_read()
than logger.record_read()
.
@nkanderson Thanks for the review and recommending the cherry-pick command. I didn't know about that command, which helped me out a ton. The PR is cleaned up now.
Regarding your comment on statistics tracking, please see below. This is recorded in docs/utils/logging-stystem.md
The logger currently requires manual tracking of statistics:
# Manual recording of operations
logger.record_read()
logger.record_write()
logger.record_hit()
logger.record_miss()
# Print statistics
logger.print_stats() # Prints to stdout by default
logger.print_stats(stream=custom_file) # Print to custom stream
The statistics tracking will be automated in a future update. The system will automatically track cache operations based on context without requiring manual recording.
# Future implementation will automatically track statistics through the decorator
@log_operation(logger=cache_logger)
def read_cache(self, address: int):
# Statistics will be automatically updated based on operation type
# and return value without explicit calls to record_*() methods
return cache_data
# Planned automatic detection for:
# - Operation type (read/write) from function context
# - Hit/miss status from return values or exceptions
# - Cache state changes from before/after operation comparison
Context Detection
Operation Categories
# Operation patterns to be automatically detected
OPERATION_PATTERNS = {
'read': ['read_cache', 'fetch', 'get'],
'write': ['write_cache', 'store', 'put'],
'hit': ['data_found', 'cache_hit'],
'miss': ['data_not_found', 'cache_miss']
}
Automatic Updates
# Example of planned automatic detection in log_operation
def log_operation(logger: CacheLogger):
def decorator(func: Callable) -> Callable:
@wraps(func)
def wrapper(*args, **kwargs):
# TODO: Implement automatic operation detection
# operation_type = detect_operation_type(func.__name__)
# pre_state = capture_cache_state()
result = func(*args, **kwargs)
# TODO: Implement automatic statistics update
# post_state = capture_cache_state()
# update_statistics(logger, operation_type, pre_state, post_state, result)
return result
return wrapper
return decorator
State Tracking
Note: Until automatic tracking is implemented, continue using the manual recording methods. The transition to automatic tracking will be backward compatible.
Added a hierarchical logging system with comprehensive unit tests for cache operations. The CacheLogger class provides three logging levels (SILENT, NORMAL, DEBUG) with different output streams for operation logs and statistics. The implementation includes a flexible decorator pattern for logging cache operations (BusOperation, GetSnoopResult, PutSnoopResult, MessageToCache), along with statistics tracking for cache hits/misses. Unit tests verify logging functionality, statistics accuracy, and file-based logging capabilities. The logger supports both console and file-based output.