d-engler
commented
1 day ago This certainly works. Just wanted to point out an alternative that is similar to the C approach. See discussion.
Closed reecewayt closed 1 day ago
d-engler
commented
1 day ago This certainly works. Just wanted to point out an alternative that is similar to the C approach. See discussion.
d-engler
commented
1 day ago Also, I'll dig into the code a little more tomorrow. Right now I'm drinking beer and doing Halloween stuff since it's my last reprieve until the end of the term...
nkanderson
commented
1 day ago @reecewayt what would you think about splitting this into 2 commits - 1 with the arg parsing and default for the filename, which you could open a PR for (and I feel like could be merged right away), and a second one with the debug parsing stuff, which can be an option in our debug discussion?
As I'm looking at this, I'm also wondering if we need / want to have a way to have a way to display the human-readable description of the operation, basically what's behind the comments in the enum in src/utils/trace_parser.py#L9-L17. I think that could be a separate issue, but it occurred to me as I was reviewing this.
reecewayt
commented
1 day ago @nkanderson Thanks for the feedback. Let me clarify a few points:
Regarding human-readable output - the enums were designed to be self-descriptive (e.g., L1_DATA_READ, SNOOPED_INVALIDATE), but if these names aren't clear enough, we can certainly revise them to be more intuitive.
I agree about moving the argument parser to a separate utility and was planning to do so.
The current implementation was primarily focused on having a working demo for next week's check-in. I'll work on making a few of these changes over the weekend.
Let me know if you have specific suggestions for the enum names or other improvements you'd like to see.
nkanderson
commented
12 hours ago Maybe we can do a PRTI so you can explain what's going on? I dunno what does everyone else think?
Haha, definitely had to look up the PRTI abbreviation, none of the teams I worked on in the past used that. But I do like mini-demos with PRs, certainly if they are complex enough. I think in this case the copy-pasted console output seems sufficient.
Regarding human-readable output - the enums were designed to be self-descriptive (e.g., L1_DATA_READ, SNOOPED_INVALIDATE), but if these names aren't clear enough, we can certainly revise them to be more intuitive
My comment was mainly because I wasn't sure what output we were required to have. I read the project description in more detail last night, and it doesn't look like we need the descriptions he provided for this particular output, so I think this is great.
I agree about moving the argument parser to a separate utility and was planning to do so.
Ah, I was actually referring to having separate commits specifically for handing the --debug option vs the --file option so we could discuss our debug plan more and not block on merging the file handling. But yeah, moving the argument parser seems reasonable too.
One thing we should consider - I think @d-engler mentioned he talked to Mark about how the file is parsed, and Mark was clear that we should not intend to read the whole file in and store the contents in memory, because it will be much too large. So we should think about what we want the initialization and file handling to look like in our main application. We could use something like a Python generator in the trace parser so the main application can just iterate over the lines with a for loop. Maybe the trace parser could even grab like 1k chunks at a time to reduce I/O operations, parse them into op, addr pairs, and yield them for whatever function is reading the data. I don't want to over complicate things, I think it would totally work to have the main application just do parser.readline(), but we could try out a generator reading chunks as an option if we have the time for it.
Completed conditional debug and user defined file feature of the file parser utility. See the usage below.