Modify background color for highlighted text when fg == bg #458

Manually merged
dnkl merged 1 commits from modify-bg-color-when-highlighted-text-has-same-fg-bg into master 7 months ago
dnkl commented 7 months ago
Owner

When rendering a selected/highlighted cell where the foreground and background colors are the same, invert the background color to make the selected text legible.

Closes #455

When rendering a selected/highlighted cell where the foreground and background colors are the same, invert the background color to make the selected text legible. Closes #455
dnkl added the
enhancement
label 7 months ago
craigbarnes reviewed 7 months ago
render.c Outdated
uint8_t g = _bg >> 8;
uint8_t b = _bg >> 0;
r = ~r; g = ~g; b = ~b;
_bg = r << 16 | g << 8 | b;
Poster
Collaborator

Could this be done as _bg = ~_bg & 0xFFFFFF? Or does the high byte have to be preserved?

Could this be done as `_bg = ~_bg & 0xFFFFFF`? Or does the high byte have to be preserved?
dnkl commented 7 months ago
Poster
Owner

Could this be done as _bg = ~_bg & 0xFFFFFF?

Absolutely...

Or does the high byte have to be preserved?

No it doesn't. Not even for alpha handling, which is done separately.

I.e. all the above can actually be simplified to bg = ~bg. This is one of those embarassing moments where you're wondering what you were thinking...

Thanks! And nice to see you back :)

> Could this be done as `_bg = ~_bg & 0xFFFFFF`? Absolutely... > Or does the high byte have to be preserved? No it doesn't. Not even for alpha handling, which is done separately. I.e. all the above can actually be simplified to `bg = ~bg`. This is one of those embarassing moments where you're wondering what you were thinking... Thanks! And nice to see you back :)
dnkl marked this conversation as resolved
dnkl force-pushed modify-bg-color-when-highlighted-text-has-same-fg-bg from 3568e08911 to d0548b2361 7 months ago
dnkl force-pushed modify-bg-color-when-highlighted-text-has-same-fg-bg from d0548b2361 to 451eab522e 7 months ago
dnkl force-pushed modify-bg-color-when-highlighted-text-has-same-fg-bg from 451eab522e to 886160ba89 7 months ago
dnkl merged commit 3131eb0288 into master manually 7 months ago
ddevault reviewed 7 months ago
}
}
if (unlikely(is_selected && _fg == _bg)) {
Poster

FYI, unlikely has not actually been useful since the 80's.

FYI, unlikely has not actually been useful since the 80's.
Poster
Collaborator

I'm intrigued to know why. Could you elaborate?

I'm intrigued to know why. Could you elaborate?
Poster

The compiler doesn't do anything with it anymore, it's a no-op. The branch predictor is a black box, there is no way for the programmer to inform it in advance on x86_64 or most other modern architectures.

The compiler doesn't do anything with it anymore, it's a no-op. The branch predictor is a black box, there is no way for the programmer to inform it in advance on x86_64 or most other modern architectures.
Poster
Collaborator

The compiler doesn't do anything with it anymore

It doesn't?

https://godbolt.org/z/sx6n9xjjn

> The compiler doesn't do anything with it anymore It doesn't? https://godbolt.org/z/sx6n9xjjn
Poster

Hm, I think this is gcc interpreting it as part of its internal branch prediction optimizer, but not giving any hints to the hardware. In any case, it does seem to do something, so I guess it works.

Hm, I think this is gcc interpreting it as part of its internal branch prediction optimizer, but not giving any hints to the hardware. In any case, it does seem to do something, so I guess it works.
Poster
Collaborator

I think this is gcc interpreting it as part of its internal branch prediction optimizer

It's GCC re-ordering the code so that the "hot" parts are contiguous, in order to lessen i-cache fragmentation. Without the likely annotation, the dynamic branch predictor will still take care of predictable branches, but it can't re-order the code to make it more cache efficient. You can only do that at compile-time.

Using __builtin_expect() in the right places can have a significant impact on performance and power efficiency.

https://easyperf.net/blog/2018/07/09/Improving-performance-by-better-code-locality#why-latter-case-is-better

(this article is a bit of a typo-fest; but illustrates the point quite well)

> I think this is gcc interpreting it as part of its internal branch prediction optimizer It's GCC re-ordering the code so that the "hot" parts are contiguous, in order to lessen i-cache fragmentation. Without the `likely` annotation, the dynamic branch predictor will still take care of predictable branches, but it can't re-order the code to make it more cache efficient. You can only do that at compile-time. Using `__builtin_expect()` in the right places can have a significant impact on performance and power efficiency. https://easyperf.net/blog/2018/07/09/Improving-performance-by-better-code-locality#why-latter-case-is-better (this article is a bit of a typo-fest; but illustrates the point quite well)
The pull request has been manually merged as 3131eb0288.
Sign in to join this conversation.
Loading…
There is no content yet.