Skip to content

make default table output a bit more compact #241

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shua
Copy link

@shua shua commented Apr 30, 2025

I found the default formatting for summarize summarize <file> to be visually noisy and difficult to read because of the separators between each line:

$ summarize summarize my-data.mm_profdata
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| Item                                                            | Self time | % of total time | Time     | Item count | Incremental result hashing time |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| LLVM_passes                                                     | 714.79ms  | 13.408          | 714.79ms | 1          | 0.00ns                          |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| run_linker                                                      | 706.56ms  | 13.253          | 706.56ms | 1          | 0.00ns                          |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| LLVM_module_codegen_emit_obj                                    | 690.58ms  | 12.954          | 690.58ms | 173        | 0.00ns                          |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| codegen_module                                                  | 401.81ms  | 7.537           | 748.49ms | 172        | 0.00ns                          |

... and so on for like 2 pages of scrolling

This change removes the separating lines +-----+---+ between rows. This introduces a new problem where it is hard to read a line as there's often a lot of whitespace between the end of the item label and the beginning of the next column. I added some ... to pad out the line to make it easier to scan.

$ target/debug/summarize summarize my-data.mm_profdata
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| Item                                                            | Self time | % of total time | Time     | Item count | Incremental result hashing time |
+-----------------------------------------------------------------+-----------+-----------------+----------+------------+---------------------------------+
| LLVM_passes ................................................... | 714.79ms  | 13.408          | 714.79ms | 1          | 0.00ns                          |
| run_linker .................................................... | 706.56ms  | 13.253          | 706.56ms | 1          | 0.00ns                          |
| LLVM_module_codegen_emit_obj .................................. | 690.58ms  | 12.954          | 690.58ms | 173        | 0.00ns                          |
| codegen_module ................................................ | 401.81ms  | 7.537           | 748.49ms | 172        | 0.00ns                          |
| incr_comp_encode_dep_graph .................................... | 302.17ms  | 5.668           | 302.17ms | 223438     | 0.00ns                          |
| implementations_of_trait ...................................... | 280.60ms  | 5.263           | 395.25ms | 44604      | 112.82ms                        |

A similar change has been made for the table output of the summarize diff ... command.

I think the final result is able to present more data in a single screen, while also not sacrificing ease of scanning horizontally.

@bjorn3
Copy link
Member

bjorn3 commented Jun 17, 2025

This doesn't seem to have any effect for me locally.
Edit: Seems like you only changed the summarize subcommand, not the diff subcommand. Please also make this change to the diff subcommand.

@shua shua changed the title make default summarize table output a bit more compact make default table output a bit more compact Jun 18, 2025
@shua shua requested a review from bjorn3 June 18, 2025 13:00
@@ -120,14 +120,25 @@ fn diff(opt: DiffOpt) -> Result<(), Box<dyn Error + Send + Sync>> {
"Incremental hashing time",
));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also use set_titles.

@@ -192,6 +203,7 @@ fn summarize(opt: SummarizeOpt) -> Result<(), Box<dyn Error + Send + Sync>> {
.sort_by(|l, r| r.self_time.cmp(&l.self_time));

let mut table = Table::new();
table.set_format(*prettytable::format::consts::FORMAT_NO_LINESEP_WITH_TITLE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing for the diff command.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

derp, I should have actually tested the diff change.

@shua shua requested a review from bjorn3 June 19, 2025 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants