X11Libre
render : use XXH3_128bits instead of SHA1 for hashing glyphs.
Cryptographic properties of SHA1 are not needed in this case, and xxHash is much faster.
Hash size changes from 20 bytes to 16 but this should not be a concern.
@alex14fr Interesting change. Could you split this up a bit? Right now, it's a single 7k line commit.
Is this a header-only hashing library that you imported? If so, can you link to the original source?
Have you done benchmarks with this?
Alternatively, If you want to keep doing *(CARD32*), you can make the hash bits a union with a CARD64(not 32, because 8 byte alignment is needed somewhere else in the code the way this is used.
Haven't reviewed yet, but in general I like the idea of switching to xxh3. I suppose we don't need a cryptographically secure hash function here. And SHA1 is old... So switching to the faster xxh3 looks quite reasonable.
@alex14fr Interesting change. Could you split this up a bit? Right now, it's a single 7k line commit.
Is this a header-only hashing library that you imported? If so, can you link to the original source?
It is a header-only library indeed at https://github.com/Cyan4973/xxHash I've included the full header, that's why it is so large.
Have you done benchmarks with this?
I've done ("manually") multiple zoom and unzoom in the st terminal emulator (https://st.suckless.org/) while recording with perf. Code is there: https://git.suckless.org/st/file/x.c.html#l295 Before patch, more time is dedicated to computing SHA1 hashes than effectively rendering the text in freetype2, with the patch this is reversed (13% to 3% of CPU time in FindGlyphRef if I remember correctly).
@alex14fr Interesting change. Could you split this up a bit? Right now, it's a single 7k line commit. Is this a header-only hashing library that you imported? If so, can you link to the original source?
It is a header-only library indeed at https://github.com/Cyan4973/xxHash I've included the full header, that's why it is so large.
Shouldn't we pull this as a dependency then, instead of copying it in our code? What happens when the library gets updated?
Including dependencies like this is a big no-no. It's also different than what's in the master repo. This should be pulled from upstream.
Having it right in the codebase without any dependencies is so nice. It’s all in one file. I couldn’t resist the temptation… It’s unlikely to require any fixes in the future after all.
Including dependencies like this is a big no-no. It's also different than what's in the master repo. This should be pulled from upstream.
I've modified so as to use xxhash as a dependency.
Looks good now. It might still be a good idea to force 8-byte alignment on the hash struct, for better speed.
@alex14fr indentation is a bit broken for some lines (with memcpy calls in particular), otherwise it looks good to me
It would be also nice to make sure it compiles on different platforms that we support.
Applied as https://patch-diff.githubusercontent.com/raw/X11Libre/xserver/pull/672.diff over master @ 2fddefcf8c73f93b3d1c69c2ca7c08b1f265ebe5 Seems like novideo's 580.76.05 driver doesn't like this patch:
(EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to
(EE) NVIDIA(0): recover...
(WW) Outdated driver still using xf86DisableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(EE) NVIDIA(GPU-0): Failed to idle DMA.
(II) NVIDIA(0): Error recovery was successful.
(WW) Outdated driver still using xf86EnableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to
(EE) NVIDIA(0): recover...
(WW) Outdated driver still using xf86DisableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(II) NVIDIA(0): Error recovery was successful.
(WW) Outdated driver still using xf86EnableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to
(EE) NVIDIA(0): recover...
(WW) Outdated driver still using xf86DisableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(II) NVIDIA(0): Error recovery was successful.
(WW) Outdated driver still using xf86EnableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to
(EE) NVIDIA(0): recover...
(WW) Outdated driver still using xf86DisableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(II) NVIDIA(0): Error recovery was successful.
(WW) Outdated driver still using xf86EnableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to
(EE) NVIDIA(0): recover...
(WW) Outdated driver still using xf86DisableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(II) NVIDIA(0): Error recovery was successful.
(WW) Outdated driver still using xf86EnableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to
(EE) NVIDIA(0): recover...
(WW) Outdated driver still using xf86DisableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
(EE) NVIDIA(GPU-0): Failed to idle DMA.
(II) NVIDIA(0): Error recovery was successful.
(WW) Outdated driver still using xf86EnableGeneralHandler() !
(WW) File a bug report to driver vendor or use a FOSS driver.
(WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
(WW) Proprietary drivers are inherently unstable, they just can't be done right.
As an experiment we could restore the 20 bytes size and fill the last 4 bytes with the same data as the first 4 bytes for example.
Yeah, it was that and it works with dgst[20]. Well for a test, I applied this 672_dgst20_first16.patch with resized dgst[16] > dgst[20], but in actual places where there's any memory op, it uses only first 16 bytes. Of course it's wrong, because this should all be done neatly with proper alignment, but with this it actually works on proprietary novideo's drivers.
@alex14fr you can rehash the 16 byte result using xxh3 64bit to get another 4 bytes. Fo our purpose it should be good enough. What do you think?
@alex14fr you can rehash the 16 byte result using xxh3 64bit to get another 4 bytes. Fo our purpose it should be good enough. What do you think?
What about setting the 4 extra bytes to zero ? This will save a hash calculation and 128 bits of digest should be sufficient to avoid collisions.
Honestly, I don’t like the idea of putting zeros there. Who knows, maybe some code uses only some bytes from the hash value and relies on them being different.
I would at least copy over some bytes from the calculated 16 bytes. Or even better do something like this:
digest[16] = digest[0] ^ digest[1]; digest[17] = digest[2] ^ digest[3]; …
Honestly, I don’t like the idea of putting zeros there. Who knows, maybe some code uses only some bytes from the hash value and relies on them being different.
I would at least copy over some bytes from the calculated 16 bytes. Or even better do something like this:
digest[16] = digest[0] ^ digest[1]; digest[17] = digest[2] ^ digest[3]; …
You're right, taking this xor looks good to me.
I think we should go back to 20 bytes everywhere for consistency. Now it's a bit strange: 16 bytes in one place and 20 in another.
@alex14fr @algrid @chankongus I still think that we should align the cache to either 4 bytes, if we want to keep exactly 20 bytes, or 8 bytres, if we can go a bit over to 24 bytes.
Do we ever use the individual bytes of the hash, or can we just do:
typedef struct {
CARD32 dgst[5];
} ExaCachedGlyphRec, *ExaCachedGlyphPtr;
?
@alex14fr @algrid @chankongus I still think that we should align the cache to either 4 bytes, if we want to keep exactly 20 bytes, or 8 bytres, if we can go a bit over to 24 bytes.
Do we ever use the individual bytes of the hash, or can we just do:
typedef struct { CARD32 dgst[5]; } ExaCachedGlyphRec, *ExaCachedGlyphPtr;?
That's a good idea. I guess the individual bytes are never used appart from this dummy XOR which can be replaced. However, I wonder if it may break ABI due to alignment issues ?
I would keep the alignment thing as a separate task. After all, the original code wasn't doing it as far as I can see. And something was clearly breaking when we tried to switch to 16 bytes, so we need to be cautious if we want to change the type of the hash.
Reapplied this over current master (which works fine as is), SDDM starts but backs out to login screen and any attempt to login again just result in resolution change(?not sure, just my TV flashes the message?) and then it kicks out to login screen again, in logs this now this segfaults the novideo 580.76.05 drivers? Weird ;/
[2025-08-22 06:39:36] Backtrace:
[2025-08-22 06:39:36] 0: /usr/lib/Xorg (xorg_backtrace+0x59) [0x5625ea7a4279]
[2025-08-22 06:39:36] 1: /usr/lib/Xorg (0x5625ea57f000+0x230c44) [0x5625ea7afc44]
[2025-08-22 06:39:36] 2: /usr/lib/libc.so.6 (0x7f4bf1e00000+0x3e240) [0x7f4bf1e3e240]
[2025-08-22 06:39:36] 3: /usr/lib/xorg/modules/drivers/nvidia_drv.so (0x7f4bf0e00000+0xb7d58) [0x7f4bf0eb7d58]
[2025-08-22 06:39:36] 4: /usr/lib/xorg/modules/drivers/nvidia_drv.so (0x7f4bf0e00000+0xa09ae) [0x7f4bf0ea09ae]
[2025-08-22 06:39:36] 5: /usr/lib/xorg/modules/drivers/nvidia_drv.so (0x7f4bf0e00000+0x51c6b6) [0x7f4bf131c6b6]
[2025-08-22 06:39:36]
[2025-08-22 06:39:36] Segmentation fault at address 0x562602f9b000
[2025-08-22 06:39:36]
Fatal server error:
[2025-08-22 06:39:36] Caught signal 11 (Segmentation fault). Server aborting
[2025-08-22 06:39:36]
[2025-08-22 06:39:36]
Please consult the XLibre support
at https://github.com/X11Libre/xserver
for help.
[2025-08-22 06:39:36] Please also check the log file at "/home/shoun/.local/share/xorg/Xorg.0.log" for additional information.
[2025-08-22 06:39:36]
[2025-08-22 06:39:36] (EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to
[2025-08-22 06:39:36] (EE) NVIDIA(0): recover...
[2025-08-22 06:39:36] (WW) Outdated driver still using xf86DisableGeneralHandler() !
[2025-08-22 06:39:36] (WW) File a bug report to driver vendor or use a FOSS driver.
[2025-08-22 06:39:36] (WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
[2025-08-22 06:39:36] (WW) Proprietary drivers are inherently unstable, they just can't be done right.
[2025-08-22 06:39:36] (II) NVIDIA(0): Error recovery was successful.
[2025-08-22 06:39:36] (WW) Outdated driver still using xf86EnableGeneralHandler() !
[2025-08-22 06:39:36] (WW) File a bug report to driver vendor or use a FOSS driver.
[2025-08-22 06:39:36] (WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148
[2025-08-22 06:39:36] (WW) Proprietary drivers are inherently unstable, they just can't be done right.
[2025-08-22 06:39:36] (EE) Server terminated with error (1). Closing log file.
Reapplied this over current master (which works fine as is), SDDM starts but backs out to login screen and any attempt to login again just result in resolution change(?not sure, just my TV flashes the message?) and then it kicks out to login screen again, in logs this now this segfaults the novideo 580.76.05 drivers? Weird ;/
[2025-08-22 06:39:36] Backtrace: [2025-08-22 06:39:36] 0: /usr/lib/Xorg (xorg_backtrace+0x59) [0x5625ea7a4279] [2025-08-22 06:39:36] 1: /usr/lib/Xorg (0x5625ea57f000+0x230c44) [0x5625ea7afc44] [2025-08-22 06:39:36] 2: /usr/lib/libc.so.6 (0x7f4bf1e00000+0x3e240) [0x7f4bf1e3e240] [2025-08-22 06:39:36] 3: /usr/lib/xorg/modules/drivers/nvidia_drv.so (0x7f4bf0e00000+0xb7d58) [0x7f4bf0eb7d58] [2025-08-22 06:39:36] 4: /usr/lib/xorg/modules/drivers/nvidia_drv.so (0x7f4bf0e00000+0xa09ae) [0x7f4bf0ea09ae] [2025-08-22 06:39:36] 5: /usr/lib/xorg/modules/drivers/nvidia_drv.so (0x7f4bf0e00000+0x51c6b6) [0x7f4bf131c6b6] [2025-08-22 06:39:36] [2025-08-22 06:39:36] Segmentation fault at address 0x562602f9b000 [2025-08-22 06:39:36] Fatal server error: [2025-08-22 06:39:36] Caught signal 11 (Segmentation fault). Server aborting [2025-08-22 06:39:36] [2025-08-22 06:39:36] Please consult the XLibre support at https://github.com/X11Libre/xserver for help. [2025-08-22 06:39:36] Please also check the log file at "/home/shoun/.local/share/xorg/Xorg.0.log" for additional information. [2025-08-22 06:39:36] [2025-08-22 06:39:36] (EE) NVIDIA(0): The NVIDIA X driver has encountered an error; attempting to [2025-08-22 06:39:36] (EE) NVIDIA(0): recover... [2025-08-22 06:39:36] (WW) Outdated driver still using xf86DisableGeneralHandler() ! [2025-08-22 06:39:36] (WW) File a bug report to driver vendor or use a FOSS driver. [2025-08-22 06:39:36] (WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148 [2025-08-22 06:39:36] (WW) Proprietary drivers are inherently unstable, they just can't be done right. [2025-08-22 06:39:36] (II) NVIDIA(0): Error recovery was successful. [2025-08-22 06:39:36] (WW) Outdated driver still using xf86EnableGeneralHandler() ! [2025-08-22 06:39:36] (WW) File a bug report to driver vendor or use a FOSS driver. [2025-08-22 06:39:36] (WW) https://forums.developer.nvidia.com/c/gpu-graphics/linux/148 [2025-08-22 06:39:36] (WW) Proprietary drivers are inherently unstable, they just can't be done right. [2025-08-22 06:39:36] (EE) Server terminated with error (1). Closing log file.
So this happens even with the 20 byte hash size?
Could you take a look at this under gdb, and show what the latest visible instruction executed is, along with a backtrace and other info?
Current version is strange: in some places it’s 20 bytes, in others it’s 16 (see GlyphRec definition for instance). We need to go back to 20 everywhere.
Current version is strange: in some places it’s 20 bytes, in others it’s 16 (see GlyphRec definition for instance). We need to go back to 20 everywhere.
Fixed :-) @chankongus Could you try again if ca1d116 solves the issue ?
Current version is strange: in some places it’s 20 bytes, in others it’s 16 (see GlyphRec definition for instance). We need to go back to 20 everywhere.
Fixed :-) @chankongus Could you try again if ca1d116 solves the issue ?
Seems to work now! Will test this further down the line and we'll see if it'll cause any problems.
EDIT: It's been stable throughout these 2 days on i3wm, but it'd be nice to test this out on several different (WM/DE)s so that any regression would be caught now rather than later. Sadly I don't have much time to do more stuff this month, so someone would need to stepup ;/
Everything looks good to me at this point, except one thing: are the changes to the meson files ok? The old removed sha1_dep was treated a bit differently. Maybe someone who has more experience with that can take a look?
Forgot to report, I tested this in Plasma and LXQT, worked fine, sadly I don't have much time this days to do stuff like this. I also run my daily driver i3wm with this patch and it doesn't cause any problems.