-
Notifications
You must be signed in to change notification settings - Fork 0
Fix cache hit statistics and show per-frame cache metrics in interactive visualizer #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -27,6 +27,51 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'heatmaps': {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CACHE_CONFIG = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'cache_size': 32768, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'line_size': 64, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'associativity': 8 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def compute_cache_stats(tracks, matrix_size): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Compute cache statistics and per-frame hit/miss data.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache = CacheSimulator( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache_size=CACHE_CONFIG['cache_size'], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| line_size=CACHE_CONFIG['line_size'], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| associativity=CACHE_CONFIG['associativity'] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| matrix_bases = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'A': 0x10000, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'B': 0x20000, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'C': 0x30000 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+44
to
+48
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hit_rate_by_frame = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hits_by_frame = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| misses_by_frame = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for a_pos, b_pos, c_pos in tracks: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr_a = matrix_bases['A'] + (a_pos[0] * matrix_size + a_pos[1]) * cache.element_size | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache.access(addr_a) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr_b = matrix_bases['B'] + (b_pos[0] * matrix_size + b_pos[1]) * cache.element_size | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cache.access(addr_b) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr_c = matrix_bases['C'] + (c_pos[0] * matrix_size + c_pos[1]) * cache.element_size | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+55
to
+61
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addr_a = matrix_bases['A'] + (a_pos[0] * matrix_size + a_pos[1]) * cache.element_size | |
| cache.access(addr_a) | |
| addr_b = matrix_bases['B'] + (b_pos[0] * matrix_size + b_pos[1]) * cache.element_size | |
| cache.access(addr_b) | |
| addr_c = matrix_bases['C'] + (c_pos[0] * matrix_size + c_pos[1]) * cache.element_size | |
| addr_a = matrix_bases['A'] + cache._get_address(a_pos[0] * matrix_size + a_pos[1]) | |
| cache.access(addr_a) | |
| addr_b = matrix_bases['B'] + cache._get_address(b_pos[0] * matrix_size + b_pos[1]) | |
| cache.access(addr_b) | |
| addr_c = matrix_bases['C'] + cache._get_address(c_pos[0] * matrix_size + c_pos[1]) |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compute_cache_stats function duplicates logic already present in CacheSimulator.simulate_accesses. Both functions iterate through tracks, compute addresses using the same formula, and call cache.access() in the same pattern (A, B, C, C). The main difference is that compute_cache_stats records per-frame statistics. Consider refactoring to avoid this duplication, perhaps by modifying CacheSimulator.simulate_accesses to optionally record per-frame statistics, or by extracting the address calculation into a shared helper function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tie stats panel to fresh simulation data
The statistics panel now reads simulation_data directly, but it is no longer updated by the same callback that recomputes tracks/cache_stats. Because Dash fires callbacks for the same input change in unspecified order, this panel can run before update_simulation and then render the previous cache stats while showing the newly selected matrix size/loop order. That stale state can persist until the user moves the frame slider again, so the panel can misreport cache performance for the current configuration. Consider wiring the stats panel to a store/output from update_simulation (or making it depend on a value that changes only after recomputation) to guarantee it uses the new data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential state mismatch between callback inputs and global state.
This callback depends on global simulation_data but is triggered by the same inputs that trigger update_simulation. If Dash processes callbacks in parallel or out of order, simulation_data may not yet reflect the new configuration when this callback runs.
Additionally, line 352 uses direct key access cache_stats['total_accesses'] which could raise KeyError if the dictionary structure is unexpected.
🛡️ Suggested defensive access
- current_hit_rate = hit_rate_by_frame[frame] if hit_rate_by_frame else 0.0
- current_hits = hits_by_frame[frame] if hits_by_frame else 0
- current_misses = misses_by_frame[frame] if misses_by_frame else 0
+ current_hit_rate = hit_rate_by_frame[frame] if frame < len(hit_rate_by_frame) else 0.0
+ current_hits = hits_by_frame[frame] if frame < len(hits_by_frame) else 0
+ current_misses = misses_by_frame[frame] if frame < len(misses_by_frame) else 0- html.P([html.Strong("Memory Accesses: "), f"{cache_stats['total_accesses']:,}"]),
+ html.P([html.Strong("Memory Accesses: "), f"{cache_stats.get('total_accesses', 0):,}"]),🤖 Prompt for AI Agents
In `@interactive_viz.py` around lines 330 - 343, This callback reads the global
simulation_data (and cache_stats) while being triggered by the same inputs as
update_simulation, risking stale/mismatched state; refactor the callback to
accept the authoritative simulation payload as a callback Input or dcc.Store
value instead of relying on the global simulation_data (referencing symbols
simulation_data and update_simulation) so it always uses the updated data, and
defensively access cache fields (e.g., cache_stats.get('total_accesses', 0) and
use .get for hit_rate_by_frame/hits_by_frame/misses_by_frame) instead of direct
indexing to avoid KeyError when the dictionary structure is unexpected
(referencing cache_stats, hit_rate_by_frame, hits_by_frame, misses_by_frame and
frame).
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential index out of bounds error when frame equals len(tracks) - 1. The check on line 336 ensures frame is at most len(tracks) - 1, but the arrays hit_rate_by_frame, hits_by_frame, and misses_by_frame could be shorter than len(tracks) if the compute_cache_stats function encounters an error or if there's any mismatch. Add bounds checking before accessing these arrays by index to prevent IndexError.
| current_hit_rate = hit_rate_by_frame[frame] if hit_rate_by_frame else 0.0 | |
| current_hits = hits_by_frame[frame] if hits_by_frame else 0 | |
| current_misses = misses_by_frame[frame] if misses_by_frame else 0 | |
| if hit_rate_by_frame: | |
| hr_index = min(frame, len(hit_rate_by_frame) - 1) | |
| current_hit_rate = hit_rate_by_frame[hr_index] | |
| else: | |
| current_hit_rate = 0.0 | |
| if hits_by_frame: | |
| hits_index = min(frame, len(hits_by_frame) - 1) | |
| current_hits = hits_by_frame[hits_index] | |
| else: | |
| current_hits = 0 | |
| if misses_by_frame: | |
| misses_index = min(frame, len(misses_by_frame) - 1) | |
| current_misses = misses_by_frame[misses_index] | |
| else: | |
| current_misses = 0 |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent handling of empty hit_rate_by_frame list. When hit_rate_by_frame is empty, line 341 sets current_hit_rate to 0.0, but if hit_rate_by_frame exists but frame is beyond its length, an IndexError will be raised. The safer pattern would be to check both if the list exists AND if frame is within bounds before accessing, similar to how hit_rate_x is handled in the visualization callback on lines 431-436.
| current_hit_rate = hit_rate_by_frame[frame] if hit_rate_by_frame else 0.0 | |
| current_hits = hits_by_frame[frame] if hits_by_frame else 0 | |
| current_misses = misses_by_frame[frame] if misses_by_frame else 0 | |
| if hit_rate_by_frame and 0 <= frame < len(hit_rate_by_frame): | |
| current_hit_rate = hit_rate_by_frame[frame] | |
| else: | |
| current_hit_rate = 0.0 | |
| if hits_by_frame and 0 <= frame < len(hits_by_frame): | |
| current_hits = hits_by_frame[frame] | |
| else: | |
| current_hits = 0 | |
| if misses_by_frame and 0 <= frame < len(misses_by_frame): | |
| current_misses = misses_by_frame[frame] | |
| else: | |
| current_misses = 0 |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential KeyError when accessing cache_stats['total_accesses']. If the cache_stats dictionary is missing the 'total_accesses' key (for example, if compute_cache_stats partially fails or if get_statistics() doesn't include it), this line will raise a KeyError. Use cache_stats.get('total_accesses', 0) for safer access.
| html.P([html.Strong("Memory Accesses: "), f"{cache_stats['total_accesses']:,}"]), | |
| html.P([html.Strong("Memory Accesses: "), f"{cache_stats.get('total_accesses', 0):,}"]), |
Copilot
AI
Jan 21, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclear variable naming: The variable hit_rate_x is constructed to represent frame indices, but the name suggests it's related to hit rate values rather than frame numbers. Consider renaming to frame_indices or sampled_frame_indices to clarify that this represents the x-axis (frame numbers) for the plot, not hit rate data.
| hit_rate_x = list(range(0, len(cache_stats.get('hit_rate_by_frame', [])), step)) | |
| else: | |
| hit_rate_x = [] | |
| cache_fig = go.Figure() | |
| cache_fig.add_trace(go.Scatter( | |
| x=hit_rate_x, | |
| sampled_frame_indices = list(range(0, len(cache_stats.get('hit_rate_by_frame', [])), step)) | |
| else: | |
| sampled_frame_indices = [] | |
| cache_fig = go.Figure() | |
| cache_fig.add_trace(go.Scatter( | |
| x=sampled_frame_indices, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing parameter documentation: The compute_cache_stats function docstring only provides a brief one-line description but doesn't document the parameters (tracks, matrix_size) or the return value structure. Given that this function returns a dictionary with extended fields (hit_rate_by_frame, hits_by_frame, misses_by_frame) beyond what get_statistics returns, the docstring should document the expected format of tracks and describe all fields in the returned dictionary.