-
Notifications
You must be signed in to change notification settings - Fork 38
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
Auto-expand tree nodes in "Data" pane #83
Conversation
works for me |
Yes, this is a superset of #82. Have you seen the example I've posted? |
Oh, the example doesn't show this well enough. Here's a better one: http://rpubs.com/krlmlr/profvis-82-2. You see that in the initial view one of the "fc" nodes is not expanded, because it's the second-heaviest on that level. |
I have two thoughts:
Using the current algorithm, it would expand the a-b-d path because a-b is 210ms, which is greater than the 150ms spent in a-e. And then it would expand a-b-d, because that has 110ms vs the 100ms for a-b-c. But if you want find the best thing to optimize, you'd probably be better off exploring the a-e path. (2) Another thought is that perhaps this intelligent expansion is a little too clever and will end up confusing users. When I look at http://rpubs.com/krlmlr/profvis-82-2, it's not obvious why that particular path is expanded, and I'm sure that users would find it mysterious. In contrast, if it just expanded lapply and FUN (I believe this is what is described in #82), then it's much more obvious why it's expanded. |
Agree to expand only if nchildren == 1. Example, slightly modified: http://rpubs.com/krlmlr/profvis-82-3. Auto-expansion doesn't seem to work always, I currently have no idea why. |
I think the reason it's not always auto-expanding is because there's a bug in the code that's causing it to lose some leaf nodes. For example, in the flame graph for http://rpubs.com/krlmlr/profvis-82-3, there are some samples with the stack So in other words, it is correctly not auto-expanding the nodes. The bug is that it's failing to display some nodes, which causes some children to appear as only children, when they actually have siblings. At least, that's what I think the problem is right now. |
After some investigation, I think that the data viewer fails to display any true leaf nodes. For example, see http://rpubs.com/wch/294531. Note that this is with the CRAN version of profvis, 0.3.3. In your previous examples, it appeared to display some true leaf nodes, but I think it's only because they had a |
The leaf node issue still needs to be fixed, but I think that's independent. |
to allow seeing the relevant details much faster.
This PR contains of two components: sorting tree nodes by time descending (commit 1), and auto-expansion (commits 2 and 3).
The auto-expansion will always expand the first node until a leaf is reached. Because the nodes are sorted by time, this will automatically focus on the most important piece of code that is likely in need of profiling.
Example run: http://rpubs.com/krlmlr/profvis-82.
Fixes #82.
Closes #50.