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

timings wrong #112

Closed
nschloe opened this issue Jun 27, 2018 · 11 comments
Closed

timings wrong #112

nschloe opened this issue Jun 27, 2018 · 11 comments

Comments

@nschloe
Copy link
Contributor

@nschloe nschloe commented Jun 27, 2018

SnakeViz tries to display the full timed call tree, but the profile file doesn't have the data for it. This leads to bogus information in SnakeViz. Consider

import time


def a(t0, t1):
    c(t0)
    d(t1)
    return


def b():
    return a(1, 4)


def c(t):
    time.sleep(t)
    return


def d(t):
    time.sleep(t)
    return


if __name__ == "__main__":
    a(4, 1)
    b()

SnakeViz gives

sv

which is wrong: c() really takes 4 seconds when called via root -> a() -> c(), and d() takes 1 second when called via root -> a() -> d(); SnakeViz shows both as 5 seconds. More timings are wrong in the above screenshot.

To work around this sort of bugs, I created tuna which gives only the information that can be deduced from the profile:

t

@jiffyclub
Copy link
Owner

@jiffyclub jiffyclub commented Jun 22, 2019

This is an important callout and such a frustrating limitation of the cProfile data. In 302fbc7 I added a note the docs describing this limitation and linking to this issue as an explainer, thanks for posting it!

@nschloe
Copy link
Contributor Author

@nschloe nschloe commented Jun 27, 2019

I don't know if this should be closed; it's certainly not fixed.

The documentation update speaks of missing information, but really SnakeViz shows incorrect information without warning. (See the example above.) I think that's pretty bad since people are just taking it for granted.

In my opinion, a documentation update doesn't cut it here; very few people will actually read it. One solution would be to remove the wrong entries from the visualization.

@jiffyclub
Copy link
Owner

@jiffyclub jiffyclub commented Jun 27, 2019

But then you wouldn't be showing the entire call tree, right?

@nschloe
Copy link
Contributor Author

@nschloe nschloe commented Jun 27, 2019

Indeed. There is no way to know the entire call tree either, that's just one property of the Python profile files.

@jiffyclub
Copy link
Owner

@jiffyclub jiffyclub commented Jun 27, 2019

Yeah, true.

sigh

@rwarren
Copy link

@rwarren rwarren commented Jun 19, 2020

FWIW, this false reporting just confused the heck out of me and caused me to spend some time trying to figure out why calls were being made that actually weren't. Oops! 🙄

Years ago I used to use runsnakerun for profiling, and I don't ever recall being bit by this, and I used it quite a lot. Was this just luck, or did runsnakerun do something trickier to infer nested timing?

FWIW, slides 8-12 in the deck for this ancient talk show an example of how runsnakerun handled nesting:
https://speakerdeck.com/rwarren/a-brief-intro-to-profiling-in-python?slide=8

I'll give tuna a try, and may also spark up a python2 venv to see how runsnakerun would have handled the example above.

@rwarren
Copy link

@rwarren rwarren commented Jun 19, 2020

FWIW, here is what runsnakerun (unfortunately python2 only) shows for the sample script in the first post of this issue:

image

There is clearly a MUCH higher priority on where time is spent vs trying to display a call tree with imperfect information available (although the call tree is still available by hovering). It still has the the same fundamental limitations due to the available data in the profile output, but it seems less obvious. Perhaps this is why I never ran into issues with runsnakerun, since the primary thing I care about is reducing time spent when doing time optimizations, not the call tree.

A squaremap visual would be an awesome addition to snakeviz!!

@pmirallesr
Copy link

@pmirallesr pmirallesr commented Jul 1, 2020

I just ran into this issue. It's pretty major in my opinion, I'm being lied to about much of the information that I actually wanted to learn about in the first place. Tuna is great at solving the false timings, but it's just much less user friendly than snakeviz (no offense). I personally don't see why this is a closed issue, am I missing something?

@nschloe
Copy link
Contributor Author

@nschloe nschloe commented Jul 2, 2020

@pmirallesr

Tuna is great at solving the false timings, but it's just much less user friendly than snakeviz (no offense)

If you could detail what you mean at https://github.com/nschloe/tuna/issues, it'd be happy to take a look.

@pmirallesr
Copy link

@pmirallesr pmirallesr commented Jul 6, 2020

@nschloe Sure! I just did, I hope you find it useful

@matkoniecz
Copy link

@matkoniecz matkoniecz commented Jul 31, 2020

I personally don't see why this is a closed issue, am I missing something?

The profiler still lies about timings, this issue was closed in commit that just documented that core functionality is broken - see 302fbc7 ("jiffyclub closed this in 302fbc7 on Jun 22, 2019 ") instead of not displaying unknown timings

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

No branches or pull requests

5 participants