Skip to content

Add print() argument to sort alphabetically#126

Merged
wch merged 3 commits into
r-lib:masterfrom
lionel-:alphabetic-sort
Jan 25, 2021
Merged

Add print() argument to sort alphabetically#126
wch merged 3 commits into
r-lib:masterfrom
lionel-:alphabetic-sort

Conversation

@lionel-
Copy link
Copy Markdown
Member

@lionel- lionel- commented Dec 2, 2020

Closes #115

Adds new print() argument sort which can take "time" (historical sorting, the default) and "alphabetical" values. The default can be changed with the profvis.sort global option. Sorting alphabetically produces the same sort of flamegraphs discussed in https://queue.acm.org/detail.cfm?id=2927301.

Edit: The print argument is now aggregate (takes a boolean) and the global option is profvis.aggregate (idem).

We might want to consider making this sorting the default as it makes it much more likely to merge the boxes within rows:

Screenshot 2020-12-02 at 15 34 45

Ideally, would be togglable from the web UI. For convenience the sorting is implemented in R, but we could generate both data frames upfront.

@wch
Copy link
Copy Markdown
Member

wch commented Dec 2, 2020

That looks nice! I really think that this needs to be done from the web UI, though.

@wch
Copy link
Copy Markdown
Member

wch commented Dec 2, 2020

Instead of alphabetical sorting, I think it would be helpful to sort by longest bar on top. I believe this is what the Data view does. It's helpful in that the things that are furthest to the left are also the most expensive (and therefore the first things to look at optimizing).

@lionel-
Copy link
Copy Markdown
Member Author

lionel- commented Dec 3, 2020

Good idea. We now sort within rows by aggregated time. Given this change, I have changed the argument from sort = c("time", "alphbetical") to aggregate = FALSE. When TRUE the new sort is used.

To make it togglable from the UI we could generate upfront the two sorts of the data and send both to the JS side. It is possible that the implementation of the Data tab could be simplified by using the data sorted on the R side. That would have the advantages of easier maintenance from R and greater unit testing coverage.

In the meantime should we change the default to TRUE? It seems like this is the most useful view of the data.

@wch
Copy link
Copy Markdown
Member

wch commented Dec 3, 2020

I'd prefer to keep the default order the same... I think the change would be confusing for people, and I do think the ordered view is useful for understanding the connection between the flow of the program and performance. This is especially true when profiling complex code. When the GUI has an obvious toggle switch for it, then users will have the best of both worlds.

@hadley
Copy link
Copy Markdown
Member

hadley commented Jan 18, 2021

Could we please try and wrap this PR and get it merged? I've already found the sorting useful in a couple of places.

@wch wch merged commit 8adb1d7 into r-lib:master Jan 25, 2021
@lionel- lionel- deleted the alphabetic-sort branch March 11, 2021 14:28
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.

Partially aggregated samples of repeated function calls

3 participants