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

Allow profiling without startup time #7648

Merged
merged 1 commit into from Jan 29, 2021
Merged

Conversation

faho
Copy link
Member

@faho faho commented Jan 23, 2021

Description

Currently, profiling a command is a bit annoying, as you need to ignore a whole bunch of extraneous information related to fish's startup.

This changes it so fish --profile only writes profiling information from after startup - anything after reading the config and before executing the postconfig commands (I'm open to suggestions for a different cut-off point), and introduces a different --profile-startup option for profiling anything before.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed
  • User-visible changes noted in CHANGELOG.rst

@faho faho changed the title Profile startup Allow profiling without startup time Jan 23, 2021
This goes to a separate file because that makes option parsing easier
and allows profiling both at the same time.

The "normal" profile now contains only the profile data of the actual
run, which is much more useful - you can now profile a function by
running

   fish -C 'source /path/to/thing' --profile /tmp/thefunction.prof -c 'thefunction'

and won't need to filter out extraneous information.
@zanchey
Copy link
Member

zanchey commented Jan 24, 2021

Sounds like a good idea though I can't comment on the implementation.

@faho
Copy link
Member Author

faho commented Jan 24, 2021

Oh the implementation is simple enough.

It's the UI I'm not completely sure about - this changes fish --profile so that it'll only profile the commands you gave it.

So before fish --profile /tmp/profile -ic 'fish_prompt' would show the profile for fish_prompt and all the loading done in config.fish and such, so you'd have ~150 lines before you even got to anything.

Now, it only shows the time for fish_prompt (including all the calls that makes and all autoloading that does), and if you want to have the startup time you'd have to pass --profile-startup /path/to/file. And if you want both, you'd have to pass both and if you want the old style of having one file you'd pass the same file for both, so it's fish --profile /tmp/profile --profile-startup /tmp/profile. My thinking was that that was rare - you typically want to know how fast your fish starts up, so you'd use --profile-startup or how fast a function is, so you'd use --profile.

The alternative is to add a --profile-mode option that you then give an argument to explain what you want profiled - --profile-mode=startup or --profile-mode=run or --profile-mode=both. Make it default to run. But that makes the common case of just profiling startup more annoying.

Or keep --profile as it is now, add --profile-startup and --profile-run to only profile one of them. This has the advantage of being strictly backwards-compatible (the current solution does repurpose an option), but it continues to leave the naive option for the wrong behavior - so if someone doesn't read the manpage closely they'd get a more awkward profile than necessary if they just want to profile a function.

@zanchey
Copy link
Member

zanchey commented Jan 24, 2021

I'd change it, it's not a common use-case and it makes more sense this way.

@faho faho added this to the fish 3.2.0 milestone Jan 29, 2021
@faho faho merged commit 594d51e into fish-shell:master Jan 29, 2021
faho added a commit that referenced this pull request Jan 29, 2021
rouge8 added a commit to rouge8/dotfiles that referenced this pull request Jan 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 29, 2021
@faho faho deleted the profile-startup branch September 23, 2021 09:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants