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

Disable syntax highlighting when pasting #5871

Closed
wants to merge 1 commit into from

Conversation

faho
Copy link
Member

@faho faho commented May 11, 2019

This adds a new pair of bind functions {en,dis}able-highlighting, that
en/dis-able highlighting, respectively.

We use these to turn off highlighting when we start bracketed paste,
and we turn it on again when we stop it.

This is by far the simplest solution to the issue of bracketed paste
being slow.

Fixes #5866.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added for regressions fixed - I have no idea how I'd test this.
  • User-visible changes noted in CHANGELOG.md

This adds a new pair of bind functions {en,dis}able-highlighting, that
en/dis-able highlighting, respectively.

We use these to turn off highlighting when we start bracketed paste,
and we turn it on again when we stop it.

This is by far the simplest solution to the issue of bracketed paste
being slow.

Fixes fish-shell#5866.
@faho faho added enhancement RFC performance Purely performance-related enhancement without any changes in black box output labels May 11, 2019
@faho faho added this to the fish 3.1.0 milestone May 11, 2019
@faho
Copy link
Member Author

faho commented May 11, 2019

I recommend testing this under valgrind. That really slows it down enough that you'll notice, especially since I have no idea how you'd measure it, since I'm not particularly keen on getting expect to do bracketed paste.

E.g. pasting the source for fish_clipboard_copy takes ~0.5s without valgrind (with callgrind), but minutes (about 1 minute per line!) with, and ~10s with valgrind and disabled highlighting. Of that, about 3s is the one highlighting pass afterwards.

It'll also show that the massive win is indeed disabling highlighting. The rest is peanuts - get_autosuggestion_performer is taking up ~98% of the time.

@zanchey
Copy link
Member

zanchey commented May 11, 2019

I think this is a good change to make, but I'm not sure that adding new bind functions is the way to go. Won't non-bracketed paste be slow too?

@faho
Copy link
Member Author

faho commented May 11, 2019

Won't non-bracketed paste be slow too?

Sure, but we can't distinguish between non-bracketed paste and entering text. That's what bracketed paste is for! Also we enable bracketed paste everywhere.

@zanchey
Copy link
Member

zanchey commented May 11, 2019

I haven't thought much about the mechanism, but would it make sense to run the highlighter on a short timer which is reset by each new keypress?

@faho
Copy link
Member Author

faho commented May 11, 2019

but would it make sense to run the highlighter on a short timer which is reset by each new keypress?

Some of our highlighting is performed on a background thread (that which performs io). But much of it isn't, and it's generally fast enough in normal usage. Also we chunk together reads if they happen quickly enough, so we already don't run highlighting on every character necessarily.

But increasing the delay to do highlighting runs the risk of making it too slow and laggy interactively, and that's the main usecase.

Really, pasting is the only problem, and for that you want bracketed paste, for multiple reasons, including pasting newlines and weird text and such. If your terminal doesn't support bracketed paste (I literally know of none), get a better terminal.

I quite like this solution, actually. It's really rather simple and it does the job.

@zanchey
Copy link
Member

zanchey commented May 11, 2019

I think I'm concerned about the introduction of more state into the keybindings functions, but I guess we already have a bunch of that for vi mode.

@faho
Copy link
Member Author

faho commented May 11, 2019

I think I'm concerned about the introduction of more state into the keybindings functions

One thing to keep in mind is that the major transition - to and from "paste" mode - is already in. That's already what happens, and it seems to work. We've not had horror stories of people getting stuck in paste mode (in which you can do nothing but insert characters, including execute. So fish would appear to hang completely).

@faho
Copy link
Member Author

faho commented May 11, 2019

Okay, I'm withdrawing this, because it's actually not necessary.

As it turns out, the issue actually is wcwidth!

Specifically ridiculousfish/widecharwidth#1: Our wcwidth goes through all checks for ASCII characters. Considering ASCII characters are about 97% of what is used in my terminal, and highlighting needs width quite a bit, that's quite a lot of time wasted.

So the solution is much simpler: adcc70d.

@faho faho closed this May 11, 2019
@ridiculousfish
Copy link
Member

Nice!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2020
@faho faho deleted the paste-no-highlight branch July 21, 2022 10:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement performance Purely performance-related enhancement without any changes in black box output RFC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance problem with pasting. fish_wcwidth at fault?
3 participants