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

Fix show all #229

Closed
wants to merge 5 commits into from
Closed

Fix show all #229

wants to merge 5 commits into from

Conversation

telamonian
Copy link
Contributor

fixes #223

This PR address the hide/show bug that @joerick pointed out in #223. I'll open separate issues for the other discussed in that issue.

This turned out to be more complex than I would have thought for rendering related junk. In part this was because the --show-all cmd line option was initially busted. Passing --show-all on the cmd line will now ensure that show_all=True gets passed to the kwargs of the Renderer's constructor. I also fixed the behavior of the base Render's constructor when show_all=True. This together appears to fix the bug pointed out in #223. @joerick Can you please test this PR out when you have a sec and let me know if that part also seems fixed to you?

- alphabetized `processors`
- added vim in devcontainer (for interactive terminal editor)
- also fixed the `show_all` behavior of the base Renderer
Copy link
Owner

@joerick joerick left a comment

Choose a reason for hiding this comment

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

This does fix the initial confusion around show_all. However, some gremlins do still exist, as seen in the output:

pyinstrument /tmp/profilet.py     main

  _     ._   __/__   _ _  _  _ _/_   Recorded: 19:04:16  Samples:  5
 /_//_/// /_\ / //_// / //_'/ //     Duration: 1.021     CPU time: 0.002
/   _/                      v4.4.0

Program: /tmp/profilet.py

1.019 <module>  profilet.py:1
└─ 1.019 A  profilet.py:16
   ├─ 0.704 D  profilet.py:3
   │  └─ 0.704 sleep  None
   │        [2 frames hidden]  ..
   └─ 0.315 sleep  None
         [2 frames hidden]  ..

Where tb-hidden frames were shown as [2 frames hidden] under the sleep.

However, I do think the change in semantics of show-all in this PR is justified.

pyinstrument/__main__.py Outdated Show resolved Hide resolved
@@ -22,40 +22,6 @@
ProcessorOptions = Dict[str, Any]


def remove_importlib(frame: Frame | None, options: ProcessorOptions) -> Frame | None:
Copy link
Owner

Choose a reason for hiding this comment

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

It's very hard for me to review this with the order of the processors changed. Could you put the functions back into the original order?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only change I made to processors here is to alphabetize the order of the functions in the file. I did this in order make it easier to enumerate the processors while figuring out what to put in the for loop in base.py.

If you don't like alphabetic I'm fine with reverting this. But I would ask that we put the functions in processors.py in some rational order

pyinstrument/processors.py Outdated Show resolved Hide resolved
Comment on lines 71 to 81
for p in (
processors.group_library_frames_processor,
processors.remove_importlib,
processors.remove_irrelevant_nodes,
processors.remove_tracebackhide,
processors.remove_unnecessary_self_time_nodes,

# always hide the outer pyinstrument cruft frames
# processors.remove_first_pyinstrument_frames_processor,
):
self.processors.remove(p)
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm. Interesting. This does highlight that the --show-all option is now deviating from the semantics of --show and --hide. However, now that we're changing it to also reveal __traceback_hide__, it does kinda make sense relative to that.

Okay, I think I agree. The only one i'd remove from above is remove_unnecessary_self_time_nodes - these were always synthetic frames, and do just clutter output in most cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove_unnecessary_self_time_nodes removed

telamonian and others added 3 commits January 17, 2023 07:16
Co-authored-by: Joe Rickerby <joerick@mac.com>
Co-authored-by: Joe Rickerby <joerick@mac.com>
@telamonian
Copy link
Contributor Author

@joerick I've addressed your comments. Mostly I just made the changes you suggested.

The output looks good with --show-all present, but without it the placement and explanation of the hidden nodes still seems kinda iffy. I think it has something to do with the grouping logic in remove_unnecessary_self_time_nodes from processors.py. I'll keep working on that. Regardless, I think this might be a good place to wrap up this particular PR, but that's up to you.

To clarify, I'm profiling this script:

import time

def D():
    time.sleep(0.7)

def C():
    __tracebackhide__ = True
    time.sleep(0.1)
    D()

def B():
    __tracebackhide__ = True
    time.sleep(0.1)
    C()

def A():
    time.sleep(0.1)
    B()

A()

For command:

pytinstrument --show-all thide.py

output:

  _     ._   __/__   _ _  _  _ _/_   Recorded: 12:28:51  Samples:  4
 /_//_/// /_\ / //_// / //_'/ //     Duration: 1.002     CPU time: 0.001
/   _/                      v4.4.0

Program: thide.py

1.002 <module>  thide.py:1
└─ 1.002 A  thide.py:16
   ├─ 0.901 B  thide.py:11
   │  ├─ 0.801 C  thide.py:6
   │  │  ├─ 0.701 D  thide.py:3
   │  │  │  └─ 0.701 sleep  None
   │  │  └─ 0.100 sleep  None
   │  └─ 0.100 sleep  None
   └─ 0.101 sleep  None

To view this report with different options, run:
    pyinstrument --load-prev 2023-01-17T12-28-51 [options]

For command:

pyinstrument thide.py

output:

 /_//_/// /_\ / //_// / //_'/ //     Duration: 1.002     CPU time: 0.001
/   _/                      v4.4.0

Program: thide.py

1.002 <module>  thide.py:1
└─ 1.002 A  thide.py:16
   ├─ 0.701 D  thide.py:3
   │  └─ 0.701 sleep  None
   │        [2 frames hidden]  <built-in>
   └─ 0.301 sleep  None
         [2 frames hidden]  <built-in>

To view this report with different options, run:
    pyinstrument --load-prev 2023-01-17T12-30-56 [options]

@mattip
Copy link
Contributor

mattip commented Apr 3, 2023

Hi. I am interested in pushing forward with this PR to get the feature in. @telamonian there is a linting issue. Can you invite me as a collaborator to your fork so I can push directly to the branch, or should I issue a new PR based on this one, cherry-picking your commits and adding one to fix the linting?

@mattip mattip mentioned this pull request Apr 30, 2023
@joerick
Copy link
Owner

joerick commented May 10, 2023

superceded by #239

@joerick joerick closed this May 10, 2023
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.

Followup for IPython integration
3 participants