Show CFGs on Nightly #608
Show CFGs on Nightly #608
Conversation
ajpal
commented
May 23, 2024
ajpal
commented
There was a problem hiding this comment.
This code requires lots of mutating the file system, so I'm getting confused about all the folders. If we can minimize the number of places we specify paths and minimize moving files around that would be great.
Besides that, making clear wrappers for mutating the state of the html page would be good
infra/nightly.sh
Outdated
| @@ -62,6 +62,9 @@ fi | |||
| # Generate latex after running the profiler (depends on profile.json) | |||
| ./infra/generate_line_counts.py "$NIGHTLY_DIR" | |||
|
|
|||
| # Generate CFGs for LLVM after running the profiler | |||
| ./infra/generate_cfgs.py "$NIGHTLY_DIR/data/llvm" | |||
There was a problem hiding this comment.
Might be better for the profile.py file to call generate_cfgs directly- less bash communication is better
There was a problem hiding this comment.
I kind of disagree? I think the separation is good- profile.py is in charge of running all the benchmarks and stuff, generate_cfgs is in charge of reading that data in and writing pngs out.
There was a problem hiding this comment.
I was hoping that having the code together would mean that there's a single place where we talk about the path to the .ll files
But it's okay if you think this is better
infra/profile.py
Outdated
| @@ -103,8 +103,9 @@ def should_have_llvm_ir(runMethod): | |||
| def dump_llvm_ir(runMethod, benchmark, output_directory): | |||
There was a problem hiding this comment.
What is the point of dump_llvm_ir? It seems to move .ll files from one folder to another. Shouldn't we just put it in that folder in the first place?
|
|
||
| async function showCFGs(benchmark, runMode) { | ||
| let pngs = ( | ||
| await fetchText(`./data/llvm/${benchmark}/${runMode}/png_names.txt`) |
There was a problem hiding this comment.
Is there a way to just fetch all pngs instead of using png_names.txt?
There was a problem hiding this comment.
Discussed offline- this is the least bad option :)
infra/nightly-resources/handlers.js
Outdated
| function copyToClipboard(eltId) { | ||
| navigator.clipboard.writeText(document.getElementById(eltId).innerText); | ||
| function toggleAllPngs(elt) { | ||
| const btns = Array.from(document.getElementsByTagName("button")); |
There was a problem hiding this comment.
This seems kinda scary if we add more buttons to the page later- perhaps just grab all stuff with a drop-down class?
There was a problem hiding this comment.
Changed to getting elements by class name
infra/nightly-resources/handlers.js
Outdated
| if (elt.innerText == "Expand All") { | ||
| elt.innerText = "Collapse All"; | ||
| btns.forEach((btn) => { | ||
| btn.innerText = btn.innerText.replace("▶ Show", "▼ Hide"); |
There was a problem hiding this comment.
This code for "what happens when you click the drop down" is duplicated between the onclick and this code. That seems not great
There was a problem hiding this comment.
I don't think there's a clean way to de-dupe them because this one is "set all to open" (or closed), while the other is "toggle" so they're not exactly the same logic
There was a problem hiding this comment.
Oh I see-
I think this could be solved by one method that sets it to open or closed. This code would set it to open. The other code would set it to the opposite of the current state
| path = f"{data_dir}/{bench}" | ||
| runmodes = os.listdir(path) | ||
| for mode in runmodes: | ||
| os.chdir(f"{path}/{mode}") |
There was a problem hiding this comment.
Sorry I didn't catch this, but I think it's bad practice to change the working directory
Can we build the relative path everywhere instead?
There was a problem hiding this comment.
Then all the dot and png files would get created in the top-level dir, which is not what we want
|
|
||
| # Delete the -init.ll file (We don't need it for nightly, | ||
| # so just reduce the amount of clutter we copy to the nightly machine) | ||
| os.system(f"rm {bench}-{mode}-init.ll") |
There was a problem hiding this comment.
Should this be deleted when it is created instead of here?
infra/profile.py
Outdated
| @@ -129,17 +123,19 @@ def aggregate(compile_times, bench_times, output_directory): | |||
| print("Usage: profile.py <bril_directory> <output_directory>") | |||
| exit(1) | |||
|
|
|||
| profile_path, output_path = os.sys.argv[1:] | |||
| # Create tmp directory for intermediate files | |||
| os.mkdir(TMP_DIR) | |||
There was a problem hiding this comment.
If this script is killed and the tmp dir is still there, does this fail? Do we want to delete it in that case?
