Skip to content
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

Merged
merged 5 commits into from
Jul 31, 2017
Merged

Auto-expand tree nodes in "Data" pane #83

merged 5 commits into from
Jul 31, 2017

Conversation

krlmlr
Copy link
Member

@krlmlr krlmlr commented Jul 25, 2017

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.

@lorenzwalthert
Copy link

works for me

@wch
Copy link
Member

wch commented Jul 26, 2017

Very nice! @krlmlr I'm glad you decided to learn enough JavaScript for this. :)

Let me make sure that I understand correctly: it is not doing the behavior described in #82 (which is what we discussed in person), but rather is expanding the "heaviest" node until it hits a leaf?

@krlmlr
Copy link
Member Author

krlmlr commented Jul 26, 2017

Yes, this is a superset of #82. Have you seen the example I've posted?

@krlmlr
Copy link
Member Author

krlmlr commented Jul 26, 2017

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.

@wch
Copy link
Member

wch commented Jul 26, 2017

I have two thoughts:
(1) Instead of expanding the heaviest child and then recursing into that, maybe it would be better to simply find the heaviest leaf node and expand to it? I'm thinking of an example where the call stack and times are something like this:

  • 100ms: a b c (that is, a calls b, and b calls c)
  • 110ms: a b d
  • 150ms: a e

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.

@krlmlr
Copy link
Member Author

krlmlr commented Jul 26, 2017

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.

@wch
Copy link
Member

wch commented Jul 27, 2017

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 lapply fun fc runif, but that sequence doesn't show up in the data viewer; it ends at fc.

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.

@wch
Copy link
Member

wch commented Jul 27, 2017

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 <GC> above them, so they weren't really leaf nodes.

@wch wch merged commit 46c7723 into r-lib:master Jul 31, 2017
@wch
Copy link
Member

wch commented Jul 31, 2017

The leaf node issue still needs to be fixed, but I think that's independent.

@krlmlr krlmlr deleted the f-#82-auto-expand branch December 11, 2017 21:54
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.

None yet

3 participants