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

Support for Font Ligatures using harfbuzz #5696

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fee1-dead
Copy link
Contributor

NOTE: alacritty/crossfont#35 has to be merged first. The current change modifies the dependency to point at my fork. It should point to a released version instead!

Closes #50. We should be able to statically link to harfbuzz on every supported platform, so not gating this behind cfg(unix).

Supersedes #2677 and #5615.

@fee1-dead
Copy link
Contributor Author

@chrisduerr I would appreciate some feedback for the current change. It is likely that not a lot of performance improvements can be made without significantly refactoring the inner Grid structure (i.e. moving shaping logic to alacritty_terminal and saving results in that structure).

@chrisduerr
Copy link
Member

It is likely that not a lot of performance improvements can be made without significantly refactoring the inner Grid structure (i.e. moving shaping logic to alacritty_terminal and saving results in that structure).

If a larger refactoring is necessary to match the current performance, then we cannot move forward without these changes. Ligatures are not worth dropping a single frame for.

@fee1-dead
Copy link
Contributor Author

fee1-dead commented Dec 24, 2021

I think we could go with an implementation similar to kitty's. They continued using cell-based rendering, except they break ligatures into individual cells that have extra data that may point to the special ligature pixmap. Text runs are not broken by the cursor or any style change, so the division is simple and not dynamic. Plus when ligatures are disabled the only overhead would be the extra bytes that each cell will take.

kitty: (each line is shaped) EDIT: there is an option to disable ligatures when cursor is over the characters, but there is no config to make color/style change break ligatures.

image

current change: (different colors are shaped individually, cursor breaks a text run into two)

image

@chrisduerr
Copy link
Member

Plus when ligatures are disabled the only overhead would be the extra bytes that each cell will take.

Since Alacritty already has dynamic storage in place for cell content that isn't essential, it should be possible to implement this without any extra bits then.

@fee1-dead fee1-dead marked this pull request as draft December 26, 2021 06:55
@fee1-dead
Copy link
Contributor Author

I just tried an implementation of this idea and it turned out to have worse performance than the current change, probably because it introduces a lot more complexity. I don't think it is worth it to try improving the performance further, because large refactorings induced headaches for me.

@chrisduerr, to minimize the regression, I'll make it so that harfbuzz is not called when the ligatures option is disabled for folks that would like a performant terminal, which should make the performance impact minimal with the option disabled (I'll test before it gets pushed). If you are still unhappy, then I can disable ligatures by default, does this sound good? This is the only way forward, at least for me, if anyone else wants to take over, I'd be happy to send a diff.

@chrisduerr
Copy link
Member

@chrisduerr, to minimize the regression, I'll make it so that harfbuzz is not called when the ligatures option is disabled for folks that would like a performant terminal, which should make the performance impact minimal with the option disabled (I'll test before it gets pushed). If you are still unhappy, then I can disable ligatures by default, does this sound good? This is the only way forward, at least for me, if anyone else wants to take over, I'd be happy to send a diff.

Having options to opt-out of performance is not acceptable. Either Alacritty manages to offer all features at high performance, or it doesn't. Just disabling all the useful features by default as an excuse to pretend like Alacritty has reasonable performance is not productive.

I strongly recommend anyone who's not willing to spend some time optimizing the ligature implementation to please stay away from sending PRs related to it. That's really just wasting the time of everyone involved.

@xsrvmy
Copy link

xsrvmy commented Jan 6, 2022

Just disabling all the useful features by default as an excuse to pretend like Alacritty has reasonable performance is not productive.

Actually, I would argue that ligatures should be off by default, because some programs might have confusing behavior with ligatures on (eg. using <= as an arrow). It would be best if ligatures can be toggled on and off, or configured to turn on automatically when certain programs are running (eg. less, vim, nvim, emacs, etc.).

@chrisduerr
Copy link
Member

Actually, I would argue that ligatures should be off by default, because some programs might have confusing behavior with ligatures on (eg. using <= as an arrow). It would be best if ligatures can be toggled on and off, or configured to turn on automatically when certain programs are running (eg. less, vim, nvim, emacs, etc.).

Oh just to clarify, regardless of what the final ligature implementation might be, it will be off by default with a configuration option to toggle it. My comment's goal was solely to point out that having options that significantly impact Alacritty's performance is not an option. Performance is gauged based on the worst-case, not the best-case scenario.

@xsrvmy
Copy link

xsrvmy commented Jan 7, 2022

@fee1-dead when running dir I saw the HT control character rendered at one point, but they fixed themselves somehow.

@Logarithmus
Copy link

Logarithmus commented Feb 3, 2022

If a larger refactoring is necessary to match the current performance, then we cannot move forward without these changes. Ligatures are not worth dropping a single frame for.

@chrisduerr Just disable ligatures by default. Those who want to use them and are OK with sacrificing a bit of FPS can enable. It's just that simple. Why are you so opposed to merging this feature?

@Logarithmus
Copy link

Logarithmus commented Feb 3, 2022

I strongly recommend anyone who's not willing to spend some time optimizing the ligature implementation to please stay away from sending PRs related to it. That's really just wasting the time of everyone involved.

Very "nice" & "supportive" attitude. Good luck getting any more pull requests to your repo. Came here after a couple of years of waiting for ligatures support. Will continue happily using kitty then.

@fee1-dead Huge thanks for your hard work, but I advise you to switch to either wezterm or kitty to avoid wasting time on this project. If the author doesn't want contributions, don't waste efforts on trying to change their view.

@chrisduerr
Copy link
Member

@chrisduerr Just disable ligatures by default. Those who want to use them and are OK with sacrificing a bit of FPS can enable. It's just that simple. Why are you so opposed to merging this feature?

Merging garbage and slow code is not an option. Either it's implemented properly or not at all. Alacritty will never get any features that opt users out of Alacritty being a reasonably quick terminal emulator, that would completely defeat the purpose of Alacritty.

@xsrvmy
Copy link

xsrvmy commented Feb 3, 2022

I strongly recommend anyone who's not willing to spend some time optimizing the ligature implementation to please stay away from sending PRs related to it. That's really just wasting the time of everyone involved.

Isn't the point of a draft PR to allow others to help?

@fee1-dead
Copy link
Contributor Author

In almost all applications or projects, eventually there will be a feature that cannot be added without a regression in performance.

If people really want the feature to be added, I have no idea why it must be "fast" such that it displays content from /dev/urandom with a minuscule font size. My proposal was to try and reduce the non-ligatures impact to zero as to give ligatures an experimental status. Clearly @chrisduerr did not like that idea because if all people end up enabling ligatures alacritty will fail to deliver the performance promise.

When developers write something in Rust, their primary focus could be performance, binary size, safety, etc. But I do not think that focus should stay forever. If it is not feature complete, it cannot be "the best terminal emulator". This project started with rather paradoxical views: (Announcing Alacritty)

Alacritty is the result of frustration with existing terminal emulators. Using vim inside tmux in many terminals was a particularly bad experience. None of them were ever quite fast enough.

Terminal emulators were not always the problem. See this reddit post where some continues to complain about nvim being very slow when run inside tmux, where the terminal emulator is alacritty! If someone tries to use nvim's terminal emulator inside tmux, it is going to be slow.

  1. Performance: Alacritty should be the fastest terminal emulator available anywhere.
  2. Appearance: Alacritty should have beautiful font rendering and look fantastic on all supported platforms.
  3. Simplicity: Alacritty should be conservative about which features it offers. As we’ve learned from past terminal emulators, it’s far too easy to become bloated.

Good appearance without the bloat nor bad performance, is, in many ways, impossible to achieve. Alacritty is in a confusing state: do you want good appearance or not? Do you want high performance or not? If you want both, then it will never be the fastest terminal emulator. Being written in Rust does not help. If you just want the high performance, then close #50 and tell everyone that this project is not going to have font ligatures because it causes regressions. Please do not continue to give people hope that it will land if some magic contributor finds a way to implement ligatures without a 2x regression in performance.

@chrisduerr
Copy link
Member

Please do not continue to give people hope that it will land if some magic contributor finds a way to implement ligatures without a 2x regression in performance.

Only because you're incapable of implementing it, does not mean it is not possible. It is most certainly possible to implement ligatures without major performance regressions.

@fee1-dead
Copy link
Contributor Author

Sorry, I retract my last sentence on that comment. I did come up with a way without regressions and that is to completely rewrite the grid storage so that it works well with ligatures (each input will only cause reshape of individual lines). It will require the effort of rewriting the entire project and is going to conflict with every other major PR.

For people who want ligatures: consider switching to another project that actually supports ligatures, but if you really want to use alacritty, you could maintain a fork.

@alacritty alacritty deleted a comment Feb 20, 2022
@Nytelife26
Copy link

I'm willing to give this a shot, and to try making it performant. No promises, but I'm willing to try. @chrisduerr, how do you suggest benchmarking the main branch and the ligatures PRs to measure progress with minimizing regressions?

@fee1-dead
Copy link
Contributor Author

Nice, @Nytelife26! For me I used MangoHud to monitor the FPS and ran cat /dev/urandom with a minuscule font size to compare the performance

@Nytelife26
Copy link

I used MangoHud to monitor the FPS and ran cat /dev/urandom with a minuscule font size to compare the performance

It appears that the preferred method (according to a blog post is to use vtebench and time cat <file>. I'll likely be using that. Anyways, at the moment I'm currently having to rebase the PR to fix merge conflicts.

Is there any chance you could add me to your fork so I can work on the PR, or should I create my own?

@chrisduerr
Copy link
Member

The recommended way to test PTY performance is by using the latest version of vtebench.

However I'd expect anyone trying to seriously verify the performance of any ligature PR to create supplementary benchmarks that stress specific worst-case scenarios. If you're struggling to come up with these, chances are you're not gonna get very far when it comes to providing a performant implementation either.

@Nytelife26
Copy link

Nytelife26 commented Apr 17, 2022

However I'd expect anyone trying to seriously verify the performance of any ligature PR to create supplementary benchmarks that stress specific worst-case scenarios.

Yeah, I get you. I'll do an additional formal series of tests, likely consisting of the following scenarios.

  • Ligatures under the cursor
  • Extended series of ligatures
  • Printing a reasonably sized source file with and without ligatures
  • Ligatures and colour boundaries combined

If you have any other suggestions, let me know, but those are the ones off of the top of my head. Should cover a lot of fairly common situations. For a more extreme stress test, I'll get it to stream out a random long block of text as fast as possible containing ligatures and then compare it to without.

Edit: I also note that there are no benchmarks in the source code. It would probably be beneficial to add some for more crucial internal functions. That way you can track regressions over time.

@chrisduerr
Copy link
Member

Edit: I also note that there are no benchmarks in the source code. It would probably be beneficial to add some for more crucial internal functions. That way you can track regressions over time.

We intentionally have no direct Rust benchmarks since they're mostly useless and do not appropriately represent the performance of the actual application.

@zenixls2
Copy link

zenixls2 commented Jun 1, 2022

zenixls2@3ed0430
this patch is fairly old, but may give you some ideas about the performance optimization.
ignore the part of performance.md. That part was estimated before all the optimization happens

just being too lazy to rebase on the latest changes in the repo.
frequent changes in the core sources, esp for the model / architecture changes makes code conflicts hard to resolve.

@chrisduerr
Copy link
Member

If a user chooses to accept a performance drop, that's fine.

This is never fine. Performance should never be opt-in or opt-out. Running Alacritty should be fast no matter what you do. A user should not need knowledge of Alacritty's internals to know which options he's allowed to use to avoid performance pitfalls.

@hughesjs
Copy link

If a user chooses to accept a performance drop, that's fine.

This is never fine. Performance should never be opt-in or opt-out. Running Alacritty should be fast no matter what you do. A user should not need knowledge of Alacritty's internals to know which options he's allowed to use to avoid performance pitfalls.

Well then you can either make it a compile-time config option so advanced users can use it, or just disable it and emit a warning if someone has it enabled and it's dropping frames.

Whilst I don't agree with your mantra of performance above all else, it's your project and you're well within your rights to take that approach. However, I think there are reasonable ways around this and I don't feel as though the project maintainers are actually trying to find a solution that works for both sides of this discussion.

@chrisduerr
Copy link
Member

Well then you can either make it a compile-time config option so advanced users can use it, or just disable it and emit a warning if someone has it enabled and it's dropping frames.

Yeah because detecting which feature of Alacritty might be causing slow downs just so we can support some slow garbage feature nobody needs is a great investment of time.

However, I think there are reasonable ways around this and I don't feel as though the project maintainers are actually trying to find a solution that works for both sides of this discussion.

I don't care about your side of the discussion. If you disagree, you can either implement it yourself or use a different terminal emulator. I have zero interest in compromising if I have nothing to gain but pain.

@kchibisov
Copy link
Member

Well then you can either make it a compile-time config option so advanced users can use it, or just disable it and emit a warning if someone has it enabled and it's dropping frames.

Compile time option could be achieved by complying the patch you want. Adding disabled by default code upstream is not a great idea.

@hughesjs
Copy link

hughesjs commented Dec 19, 2023

Yeah because detecting which feature of Alacritty might be causing slow downs just so we can support some slow garbage feature nobody needs is a great investment of time.

Dude, it's objectively not a garbage feature when clearly many, many people want it. If you don't want to invest your time on it though, that's fair.

I don't care about your side of the discussion. If you disagree, you can either implement it yourself or use a different terminal emulator. I have zero interest in compromising if I have nothing to gain but pain.

Like this is fair enough, although we're having this discussion on a PR, so someone clearly has implemented it. So my question then becomes what objective measure needs to be met where you would actually consider merging ligature support? Obviously, I've not read the entire thread on this because it's huge, so sorry if it's already been said, but literally no performance loss is pretty much impossible when adding any feature. So what's the benchmark? It's fast enough that we don't drop any frames on modern hardware? 10 year old hardware? What frame-rate are we assuming the display has?

I'd be willing to have a crack at this when I get the time if I knew what the goalposts were and that if I could get them met then it would be merged.

If the answer is that you never would, then alright, just close this PR and indicate as such on the locked thread that you don't want ligatures in alacritty and never will.

Compile time option could be achieved by complying the patch you want. Adding disabled by default code upstream is not a great idea.

I don't know if that's true, many, many common linux tools have code where you selectively compile what features you want. Fair enough if you don't want to go down that route though, I could fairly easily make an AUR entry with this patch (if someone hasn't already).

@kchibisov
Copy link
Member

I'd be willing to have a crack at this when I get the time if I knew what the goalposts were and that if I could get them met then it would be merged.

The throughput should stay the same, frame rate could drop only slightly (a few frames) when the shaping is actually performed, and the input lag should not change at all.

It also should work just fine on low end CPUs, like arm ones found in pine64 hardware where CPUs are really slow.

In general, this issue requires a lot of effort in making it fast.

Be aware that in environments like browsers where massive blocks of text are static it's not really an issue, because this content is shaped once, but in terminal where everything can change at any time, it's more complex.

Like I know how to make it the same perf (in margin of error) when the user just edits text (and it has a lot of exceptions here as well, because text overlapping). But when your whole screen is redrawn it gets way more complicated.

@hughesjs
Copy link

hughesjs commented Dec 19, 2023

Would a solution whereby ligatures are only drawn when the screen has been static for a small amount of time be acceptable (on the order of ms)? Or maybe they get temporarily disabled when throughput is greater than a certain amount. I don't think anyone would mind too much if ligatures aren't drawn when you've got huge amounts of text scrolling and the screen is being constantly redrawn. That might alleviate some of the performance concerns?

@kchibisov
Copy link
Member

Would a solution whereby ligatures are only drawn when the screen has been static for a small amount of time be acceptable (on the order of ms)?

This is just poor UX, at this point just use conceal in vim if you really want them.

@hughesjs
Copy link

Is it? I would hope you could get it working so that any delay after scrolling was essentially imperceptible. I don't know about you but when I have enough text scrolling through my terminal that performance becomes an issue, I can't really follow what's happening with any level of clarity. Maybe I'm underestimating how much throughput would cause a problem though

@fee1-dead
Copy link
Contributor Author

some slow garbage feature nobody needs

I think it is important to remind people in this discussion that ligatures isn't just some fancy eye candy for programmers. Many languages require ligatures in order to display correctly. See this for an overview of this topic. Many other issues would be resolved with ligatures (#936 #3975 #4025 just to mention a few) but the maintainers simply thinks having full unicode support is "garbage".

Of course, why care about internationalization at all when your only goal is to cling on to the performance? Sorry, you guys can keep on talking about "blazingly fast software" and "fastest terminal emulator" but I'm never going to use it if it has literally inferior font rendering.

@kchibisov
Copy link
Member

#3975 can't be done without breaking clients, needs negotiation, has nothing to do with ligatures, just shaping.

#936 breaks width as well, can't be done without negotiation with client.

#4025 similar.

All you've linked has nothing to do with ligatures though, ligatures preserve the width, the modifiers don't preserve the width, it's a completely different issue and fixing ligatures won't fix any of that, but ofc you know better.

@fee1-dead
Copy link
Contributor Author

Why do both maintainers like doing things like calling other developers incompetent and saying things like "ofc you know better"? I used the phrase 'ligatures' with an intention for it to mean text shaping, and that is what is done in this PR. If you're going to come up with a way to implement ligatures without getting the benefits of text shaping, you can feel free to go for it. I have not looked at this code for more than a year, and if the width issue exists, it should still be an easy fix.

But really who cares about an insignificant technicality? From my point of view the amount of toxicity displayed is considerably off-putting for any FOSS project, let alone a Rust one.

@kchibisov
Copy link
Member

but the maintainers simply thinks having full unicode support is "garbage".

Why do both maintainers like doing things like calling other developers incompetent and saying things like "ofc you know better"?

You decided speak for us, you've got reply based on what you wrote. If it offended you, then sorry, but I'm not sure what you've expected from commenting that way. If you disagree you can just move on.

If you're going to come up with a way to implement ligatures without getting the benefits of text shaping, you can feel free to go for it.

The point is that text shaping similar to what ligatures are doing only works for input that doesn't change width. And I'm not familiar with anything other than ligatures which preserve width. Also, none of the PRs I've tested with ligatures made country flags work, though I don't remember testing it on this PR, since I closed it after massive perf loss.

and if the width issue exists, it should still be an easy fix.

Width between client and terminal, it's not a rendering sort of thing (unless you generally want to center glyphs within an area they're present). There're some term specific escapes which do negotiation on how both terminal and client calculate them, it's not an easy fix as you say.

But really who cares about an insignificant technicality?

width is clearly not insignificant, it makes the tui app unusable if you do mismatch, because the cursor can't be positioned reliably anymore and you have whole screen of artifacts.

From my point of view the amount of toxicity displayed is considerably off-putting for any FOSS project, let alone a Rust one.

Yes, not willing to accept every patch in open source is very toxic, I understand, and also getting constant spam on issue telling us how bad we are that we don't accept every patch we don't like.

And when it comes to ligatures PRs it's like the 4-th? iteration of nearly the same code, what would you expect where all solution are basically the same as the first suggested one and which had proper reviews, etc?

@kchibisov
Copy link
Member

kchibisov commented Dec 19, 2023

And to be clear, I've rechecked the PR and it's 4x slower and none of the linked issues (which are not ligatures) are handled, so flags are broken the same way, etc. Only said eye candy is rendered now.

Edit: I'm not expecting linked issues to be fixed with ligatures work, it's like you're speaking from the point is that your patch is going to fix all the unicode.

@fee1-dead
Copy link
Contributor Author

Yes, not willing to accept every patch in open source is very toxic, I understand, and also getting constant spam on issue telling us how bad we are that we don't accept every patch we don't like.

I don't think you got what I meant at all.

@hughesjs
Copy link

hughesjs commented Dec 19, 2023

Putting the above comments about toxicity aside, as I think everyone involved here could benefit from taking some of the heat out of this discussion.

Is there a script anywhere that runs some objective performance tests that we can use to set a goal? Perhaps, we could define a VM that's suitably specced for a reproducible run? Then we could even look to add a performance test as a check to the pipelines and it gives all PRs an objective performance measure they can work to.

@kchibisov
Copy link
Member

I don't think you got what I meant at all.

I've got what you mean, but behavior on every ligature relating issue is toxic from both sides from the start, so not sure what you really wanted. And for me your behavior was toxic from the start, it's all subjective in the end, especially when you directly starting to call out.

And some necrobumping.

Sorry, I retract my last sentence on that comment. I did come up with a way without regressions and that is to completely rewrite the grid storage so that it works well with ligatures (each input will only cause reshape of individual lines). It will require the effort of rewriting the entire project and is going to conflict with every other major PR.

I think it was even possible back then when you wrote it, since you have damage information through out the terminal, and you can shape the individual lines, and even do partial redraws (I have PR around to do that, which could resurected). Which is the reason why I said now that I know how to do it for regular common case to be nearly the same as master, but not for full grid change, when e.g. scrolling.

It's not like you wanted to discuss the implementation (though, I wasn't around that time due to issues, so maybe @chrisduerr remembers you comming to irc and asking something).

Besides.

It's not like we're not interested at adding shaping at all, it's just requires work and probably communicating with us, and be aware that alacritty is just one of the projects we're doing in our free time, besides full time jobs and other issue. If you have challenges during implementation you can ask on irc (I generally reply to anyone, and you can even make me write patch for you if it's damage/partial rendering related, since I'm interested in that). But just claiming that you know how, but it'll require a rewrite and you won't do that is not productive, for both you and us. Maybe an idea to just make a rewrite of something internally besides the ligatures is not bad in the end, given that some parts of our grid code is just garbage.

And it's the reason why @chrisduerr said that

I strongly recommend anyone who's not willing to spend some time optimizing the ligature implementation to please stay away from sending PRs related to it. That's really just wasting the time of everyone involved.

@kchibisov
Copy link
Member

Is there a script anywhere that runs some objective performance tests that we can use to set a goal?

If you touch the terminal related code (the part behind the mutex in display/mod.rs) vtebench is enough, however for rendering we don't have great infra other than disabling internal frame throttling and using things like GALLIUM_HUD=fps (which may not always be representative), you can also profile rendering time by measuring time you waste to produce a frame (similar to what our render timer is doing (it accounts for GL calls mostly, so could be not that accurate, but it gives some base).

@fee1-dead
Copy link
Contributor Author

I've got what you mean, but behavior on every ligature relating issue is toxic from both sides from the start, so not sure what you really wanted. And for me your behavior was toxic from the start, it's all subjective in the end, especially when you directly starting to call out.

I'm sorry about the confrontational tone in some of my last comments. I will try my best to be more productive here.

I think a lot of the optimizations can be done by reducing the amount of data we feed to harfbuzz. I've read into the documentation again, and the cluster information (provided to us through GlyphInfo when shaping through harfbuzz_rs) can be very useful. I'm thinking some of the following things:

  1. when resizing, we can look at the clusters to determine if any of the ligatures were broken up. If they were (if the line breaks a sequence with the same cluster value), we reshape, if not, we simply reuse the existing information. (might also need to take into account the unsafe_to_break flag)
  2. when a specific location's text changed, we might simply be able to only use the neighboring clusters for partial reshaping. I'm not entirely sure about whether this is acceptable, because I haven't worked closely with shaping code much, but the last few paragraphs on this page seems to suggest that is is okay, as long as we supply it with context.
  3. Caching a shaping plan for the different font faces to avoid the planning cost for each shape run. The safe wrapper doesn't seem to support this, so harfbuzz_sys should probably be used.

@hughesjs
Copy link

Is there a script anywhere that runs some objective performance tests that we can use to set a goal?

If you touch the terminal related code (the part behind the mutex in display/mod.rs) vtebench is enough, however for rendering we don't have great infra other than disabling internal frame throttling and using things like GALLIUM_HUD=fps (which may not always be representative), you can also profile rendering time by measuring time you waste to produce a frame (similar to what our render timer is doing (it accounts for GL calls mostly, so could be not that accurate, but it gives some base).

Maybe this would be a useful starting point for everyone then, not just this issue. Performance is clearly very important to you, but it's also quite a nebulous thing in a terminal. So, perhaps if we work together to come up with an easily reproduceable and rerunnable testbench, you can set it as an AC on all PRs that everyone can understand.

You up for that? If so, I can create a separate issue for it and take that discussion over there and I might even have a chance to look at it over the christmas break.

@kchibisov
Copy link
Member

when resizing, we can look at the clusters to determine if any of the ligatures were broken up. If they were (if the line breaks a sequence with the same cluster value), we reshape, if not, we simply reuse the existing information. (might also need to take into account the unsafe_to_break flag)

Resize could actually be fairly complex to optimize (and not sure whether it's worth it)l, given that with reflow involved grid could require attaching rendering metadata to it (but it's separate from the rendering), and in general it could require grid rewrite (it's not reallyl a bad thing, given that the current structure struggles a bit), since
we could benefit from metadata as well to allow users attach content they want to reflow.

when a specific location's text changed, we might simply be able to only use the neighboring clusters for partial reshaping. I'm not entirely sure about whether this is acceptable, because I haven't worked closely with shaping code much, but the last few paragraphs on this page seems to suggest that is is okay, as long as we supply it with context.

This is all tracked through out alacritty/alacritty_terminal now (was before, but now with a bit better abstractions), so that thing sounds possible and what I was thinking of doing. Like we have information on what roughly changed within each line, so we can try to reshape based on that data, which will reduce the amount we're wasting by a lot. The challenge is to store clusters somehow, but I think it's a sort of thing you get only when you try to plumb it.

My main issue is how to deal with full-screen updates and scrolling.

Probably scrolling could be handled with damage information we already have if we try to rotate it inside the viewport, surely we still need to re-damage all the frame, but the information on what was changed inside the lines could be more or less accurate here (less accurate means overdamaging).

As for full-screen updates I don't have an answer, since I never tried shaping and don't know common patterns. Maybe something like Trie could help, but I'm not sure that it could actually work here, other than probably clusters have a common start, etc. And I'm also not familiar with harfbuzz much, since ENOTIME to look myself...

@kchibisov
Copy link
Member

So, perhaps if we work together to come up with an easily reproduceable and rerunnable testbench, you can set it as an AC on all PRs that everyone can understand.

@hughesjs we already have https://github.com/alacritty/vtebench and https://github.com/alacritty/termbenchbot (and yes, you can use it to trigger perf report on alaritty PRs).

What we don't have is automated rendering performance reporting, which could be nice and I'll be happy to review a patch adding instrumentation to collect fps more reliably out of alacritty (behind a compilation flag, which will be used by said bot).

@kchibisov
Copy link
Member

As for full-screen updates I don't have an answer, since I never tried shaping and don't know common patterns. Maybe something like Trie could help, but I'm not sure that it could actually work here, other than probably clusters have a common start, etc. And I'm also not familiar with harfbuzz much, since ENOTIME to look myself...

Seems like clusters doing something similar if I read the links correctly, at least they can sort of determine the mappings.

@kchibisov
Copy link
Member

Oh, and besides what was said.

I think the good way to approach that issue would be to just start outside of alacritty and try to shape grid (basically Vec<Vec<char>> like we have) of text with various techniques and compare perf when just using freetype vs freetype + harfbuzz shaping. And do that for different patterns of grid updates. The rendering could be omitted all together, since it's not relevant for what we're trying to benchmark, and drawing a little bit larger glyph shouldn't be a problem.

That should be more productive or at least educational, since when doing directly inside alacritty it'll likely just prevent from any progress and one could start fighting the codebase. And as for maintainer it'll really help to see why particular approach was used, when you have benchmarks to show it's easier to convince people.

Doing it separately could also benefit others, since they can pick up/play with what was discovered.

@hughesjs
Copy link

we already have https://github.com/alacritty/vtebench and https://github.com/alacritty/termbenchbot (and yes, you can use it to trigger perf report on alaritty PRs).

Does it run them on virtual hardware approximating the lowest-power device you'd like to be able to run at full speed? I think that's the key here

@kchibisov
Copy link
Member

Does it run them on virtual hardware approximating the lowest-power device you'd like to be able to run at full speed? I think that's the key here

The point is delta, it's all on real hardware, since hardware acceleration, but the system is powered on, run this benchmark, and powers back off. It's sole purpose is to run the benchmarks.

@hughesjs
Copy link

Does it run them on virtual hardware approximating the lowest-power device you'd like to be able to run at full speed? I think that's the key here

The point is delta, it's all on real hardware, since hardware acceleration, but the system is powered on, run this benchmark, and powers back off. It's sole purpose is to run the benchmarks.

Ahh fair, that'll probably do... I was just thinking less modern hardware might be hit harder than more mordern hardware for certain feature changes, so the delta might not be linear

@chrisduerr
Copy link
Member

I was just thinking less modern hardware might be hit harder than more mordern hardware for certain feature changes, so the delta might not be linear

That's why it's running an ancient GPU.

@fee1-dead
Copy link
Contributor Author

in general it could require grid rewrite

and supporting text shaping where the glyphs' widths change, as you said, would probably require a rewrite as well? But that should probably be after harfbuzz is introduced to the codebase first, right?

try to shape grid (basically Vec<Vec<char>> like we have) of text with various techniques and compare perf when just using freetype vs freetype + harfbuzz shaping. And do that for different patterns of grid updates

I'm not sure if I understand what you meant here. Does alacritty/alacritty_terminal supply grid updates as some sort of Event enum that tells you what needs to be changed?

@kchibisov
Copy link
Member

and supporting text shaping where the glyphs' widths change, as you said, would probably require a rewrite as well? But that should probably be after harfbuzz is introduced to the codebase first, right?

They just can't be supported, regardless of grid, because you'll break client width as well, thus the entire screen will be unreadable. There're experimental escapes to negotiate width, but that's about it.

Does alacritty/alacritty_terminal supply grid updates as some sort of Event enum that tells you what needs to be changed?

It basically gives iterator and we also have a Vec<LineDamage> telling what changed in lines, it's at least enough to understand which lines changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Support for ligatures