Fix piping to Helix on macOS (pending Crossterm update)

[yyogo]

Crossterm will soon merge https://github.com/crossterm-rs/crossterm/pull/735 which will let it use /dev/tty on macOS. This will fix #2111 and allow piping to Helix on macOS.

[kirawi]

Waiting on a crossterm release with the meged PR.

[archseer]

What are the performance implications here? I figured the mio one is probably better optimized

[yyogo]

What are the performance implications here? I figured the mio one is probably better optimized

Fwiw, I haven't noticed any performance differences in Helix or other projects using crossterm when testing manually.

I did mean to do a comparative benchmark for the crossterm PR, but gave up since I couldn't figure out how to do that without emulating the controlling terminal. If there are any existing benchmarks for helix, or if you know of a similar suite in another project, I could try to run it and report what I find.

[archseer]

A flame graph before/after the change would be useful. As a really simple test you could try opening a 10MB text file and scrolling from top to bottom. Measuring the flame graph there + time taken would be at least one data point

[danriedl]

What is left to merge this PR? The test you mentioned @archseer ?

[David-Else]

@danriedl I think there is talk of using a git dependency as a temporary measure for some essential fixes before 0.26.1 https://github.com/helix-editor/helix/pull/4939

[the-mikedavis]

I scrolled through Moby Dick with touchpad scrolling as fast as possible with this and master at 1a87d14439bc940d9bf3e66359a612b345aa363f

The flamegraphs look pretty similar and they both took around 35s. Master:

This PR:

[pascalkuthe]

Actually crossterm does look like its taking up a bit more space in that flamegraph although not by a huge amount. Probably doesn't matter much in practice.

Couldn't we just turn this feature on for macos and keep it off for other platforms? Platform specific feature are possible with cargo I think. On macos the extra functionality is definitly worth whatever small performance regression this might cause and on other platforms nothing would change then

[archseer]

The one by this PR actually looks shorter though? There's some mentions about enabling this feature and dropping mio so it might not make sense to have it conditionally enabled in the long run

[yyogo]

@the-mikedavis sorry I couldn't get to this so far, thank you for taking the time. Please see https://github.com/crossterm-rs/crossterm/issues/755 - I think we should hold off on this pr until it is fixed (I'm working on it).

[the-mikedavis]

Looks like there's a new crossterm 0.26.1 release with your fix 🚀 https://github.com/crossterm-rs/crossterm/releases/tag/0.26.1

[pascalkuthe]

The one by this PR actually looks shorter though? There's some mentions about enabling this feature and dropping mio so it might not make sense to have it conditionally enabled in the long run

Hmm yeah I saw the same difference but switched the two plots up by accident. This seems very surprising to me tough. mio is really well optimized (and we depend on it anyway trough tokio). If I understood the Pr that integrates tis into crossterm right there is some trickery going on to create a waker. I would have assumed that caused overhead. Just out of curiosity. On which operating system was that flamegraph recorded @the-mikedavis?

[the-mikedavis]

That was a macOS laptop. I'll take some linux ones too for comparison.

---

Ok here's Linux on 1a87d14 scrolling through Moby Dick by holding C-f (trackpad scrolling is working differently between macOS and Linux so I recorded all of these by holding C-f instead):

Linux on this PR:

MacOS on 1a87d14:

MacOS on this PR:

[pascalkuthe]

Thanks for the benchmarks! Interesting how different linux and mac are. Is it possible you switched the flamegraphs up in your earlier comment? It looks like this PR is a slight performance regression on both macos on linux from the new flamegraphs you posted. The new macos benchmarks look similar to your earlier benchmark but swapped and now looks roughly what I would have gussed.

It doesn't seem like crossterm will switch away from mio immedietly, the followup PR (https://github.com/crossterm-rs/crossterm/pull/743) still has everything behind a flag (and doesn't drop mio from the dependencies either). I think for now keeping it behind a cfg would make sense as its a performance regression and needs one extra dependency. It doesn't matter much either way tough.

[the-mikedavis]

Ah yeah, they weren't recorded very scientifically either, just swipe scrolling 😅. I don't have the original files around so I can't be sure

[heliostatic]

I'm running into this crash on a Mac with crossterm 0.26.1 (so with @yyogo 's crossterm patch). How do I properly configure my build of helix to use tty? Right now I'm crashing trying to use fzf to find an execute helix, e.g. fzf --bind 'enter:become(hx {})'

[pascalkuthe]

I'm running into this crash on a Mac with crossterm 0.26.1 (so with @yyogo 's crossterm patch). How do I properly configure my build of helix to use tty? Right now I'm crashing trying to use fzf to find an execute helix, e.g. fzf --bind 'enter:become(hx {})'

this PR wasn't merged yet. Crossterm doesn't use dev-tty unless you explicitly set a feature flag

[the-mikedavis]

(Pushed a merge commit since I made a bunch of conflicts in #4939 😅)

[diegs]

@the-mikedavis as you pointed out crossterm 0.26.1 was released a few months ago with the crash fix. What are the remaining decision criteria for switching to the dev-tty feature, at least on MacOS? Are the performance implications too concerning? Thanks!

[yyogo]

We could also enable the feature only on macos, I would think the usability improvement is worth the slight performance regression. ~~(actually I'm not sure if cargo lets you do this: https://github.com/rust-lang/cargo/issues/1197)~~ edit: turns out it is possible

[yyogo]

~~Closing this in favor of https://github.com/helix-editor/helix/pull/7607.~~