Refactor traceAnalyzer to improve output handling and add popularity …#310
Refactor traceAnalyzer to improve output handling and add popularity …#310
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the traceAnalyzer to improve output handling and adds a popularity decay option. It also includes improvements to the CLI argument parsing and fixes a bug related to empty trace files. The changes aim to make the tool more robust and feature-rich. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the traceAnalyzer to improve output handling, adds a new popularityDecay option, and fixes a few issues. The output file paths are now configurable via the --output command-line argument, and empty traces are handled gracefully. A bug in request capping logic is fixed in the trace reader. The run_traceAnalyzer.sh script is significantly improved for robustness and correct argument handling. My review includes suggestions to remove redundant file output and to clean up the format of statistics for better readability.
| ofstream ofs(output_path_ + ".stat", ios::out | ios::app); | ||
| ofs << gen_stat_str() << endl; | ||
| ofs.close(); |
There was a problem hiding this comment.
The main function already writes the summary statistics (from gen_stat_str()) to a .traceStat file and to standard output. This code block duplicates that by writing the same information to a .stat file. To avoid redundancy and keep output handling centralized in main, consider removing these lines. The dump_outputs function would then be responsible only for dumping the detailed statistics from the individual analyzer components (like ttl_stat_, req_rate_stat_, etc.).
| << "write: 0(0.0000), overwrite: 0(0.0000), del:0(0.0000)\n" | ||
| << "X-hit (number of obj accessed X times): "; | ||
| for (int i = 0; i < track_n_hit_; i++) { | ||
| stat_ss_ << "0(0.0000), "; |
There was a problem hiding this comment.
This line adds a trailing comma and space to the output string (e.g., 0(0.0000), 0(0.0000), ). To produce cleaner output, it's better to avoid adding the separator after the last element. A similar issue exists in the loop for track_n_popular_ below, and also in the loops that handle the case when n_req_ > 0.
stat_ss_ << "0(0.0000)" << (i < track_n_hit_ - 1 ? ", " : "");|
|
||
| stat_ss_ << "freq (fraction) of the most popular obj: "; | ||
| for (int i = 0; i < track_n_popular_; i++) { | ||
| stat_ss_ << "0(0.0000), "; |
There was a problem hiding this comment.
Similar to the loop for track_n_hit_, this line adds a trailing comma and space. It's better to avoid this for cleaner output.
stat_ss_ << "0(0.0000)" << (i < track_n_popular_ - 1 ? ", " : "");There was a problem hiding this comment.
Pull request overview
Refactors the traceAnalyzer tooling to improve output handling (output prefix-based filenames), add/repair popularity-decay CLI support, and harden wrapper/script behavior around running the analyzer.
Changes:
- Update
traceAnalyzerto dump stats and analysis outputs more consistently (including empty-trace handling) and gate popularity post-processing on non-empty traces. - Fix/extend CLI parsing (rename
popularityDacay→popularityDecaywith hidden backward-compatible alias, require--outputarg, add--track-n-popular, simplify--verbose). - Improve the
run_traceAnalyzer.shwrapper (safer bash settings, binary path resolution, quoting).
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/run_traceAnalyzer.sh | Adds strict bash mode, robust pathing, and refactors how analyzer args are assembled. |
| libCacheSim/traceReader/reader.c | Adjusts request-cap logic to correctly honor cap_at_n_req values (including 0/1). |
| libCacheSim/traceAnalyzer/analyzer.cpp | Centralizes output dumping, adds empty-trace handling, and avoids popularity processing on empty maps. |
| libCacheSim/bin/traceAnalyzer/main.cpp | Writes traceStat output using the configured output prefix rather than a fixed filename. |
| libCacheSim/bin/traceAnalyzer/cli_parser.cpp | Fixes/extends CLI options (popularityDecay spelling + alias, output arg semantics, verbose flag behavior, track-n-popular parsing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ofs << gen_stat_str() << endl; | ||
| ofs.close(); |
There was a problem hiding this comment.
Changing the stats output filename from a fixed stat to output_path_ + '.stat' is a user-visible behavior change. Existing docs and any workflows that look for stat in the working directory will no longer find it. Either update the documentation/plotting entrypoints accordingly in the same PR, or consider emitting the legacy filename as well for backward compatibility.
| ofs << gen_stat_str() << endl; | |
| ofs.close(); | |
| ofstream legacy_ofs("stat", ios::out | ios::app); | |
| auto stat_str = gen_stat_str(); | |
| ofs << stat_str << endl; | |
| legacy_ofs << stat_str << endl; | |
| ofs.close(); | |
| legacy_ofs.close(); |
| analyzer_args+=("--trace-type-params" "$*") | ||
| fi | ||
|
|
There was a problem hiding this comment.
The script now treats all extra arguments as a single value for --trace-type-params (analyzer_args+=('--trace-type-params' "$*")). This prevents callers from passing any other traceAnalyzer flags (e.g., --output, --time-window, --popularityDecay, etc.), and also breaks if the user already supplies -t/--trace-type-params in the extra args (it would become part of the params string). Consider forwarding remaining args verbatim (e.g., append "$@") and only adding --trace-type-params when you explicitly want to support a third positional parameter, or provide a dedicated flag in the script for trace-type params.
| analyzer_args+=("--trace-type-params" "$*") | |
| fi | |
| # Treat the third positional argument as trace-type parameters | |
| analyzer_args+=("--trace-type-params" "$1") | |
| shift 1 | |
| fi | |
| # Forward any remaining arguments verbatim to traceAnalyzer | |
| if [ $# -gt 0 ]; then | |
| analyzer_args+=("$@") | |
| fi |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af4073d3b9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,5 +1,7 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -euo pipefail | |||
There was a problem hiding this comment.
On traces with fewer than 200 distinct objects, Popularity::dump() returns without creating *.popularity (libCacheSim/traceAnalyzer/popularity.cpp:18-23,55-60), and scripts/traceAnalysis/popularity.py immediately open()s that file. Turning on set -e here makes scripts/run_traceAnalyzer.sh exit non-zero for those valid small traces (or capped runs), even though the analyzer has already produced the other outputs; before this change the plotting phase was effectively best-effort.
Useful? React with 👍 / 👎.
| analyzer_args=("${tracepath}" "${trace_format}" "--common") | ||
| if [ $# -gt 0 ]; then | ||
| analyzer_args+=("--trace-type-params" "$*") |
There was a problem hiding this comment.
This now rewrites every argument after <trace_format> into a single --trace-type-params string. As a result, previously valid invocations like scripts/run_traceAnalyzer.sh ... --num-req 100000, --output out/prefix, or --popularityDecay no longer reach traceAnalyzer; they are handed to parse_reader_params(), which exits on unknown keys (libCacheSim/bin/cli_reader_utils.c:183-185). That breaks capped analyses and custom output prefixes through the helper script.
Useful? React with 👍 / 👎.
…decay option