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.

Sign up for GitHub

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

Jump to bottom

write-qt-apps-hook.sh: use make-binary-wrapper for significant speedups #225881

Merged
merged 1 commit into from
Apr 16, 2023
Merged

write-qt-apps-hook.sh: use make-binary-wrapper for significant speedups #225881

merged 1 commit into from
Apr 16, 2023

Conversation

matklad
Copy link
Member

@matklad matklad commented Apr 12, 2023

Description of changes

QT apps tend to call makeWrapper with a lot of arguments, which causes noticable slowdowns (+100ms for app startup). The slow down boils down to two reasons:

  • the required string processing is O(N^2)
  • bash is slow at doing the processing

By using the binary wrapper, we fix the second point, brining the overhead down from 100ms to just 4ms or thereabouts.

I tested this change by rebuilding my whole system with it (I use plasma). It booted and it works (that's where I type this commit message), but I am not 100% sure if this won't break anything else.

Closes: #225871

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@matklad
write-qt-apps-hook.sh: use make-binary-wrapper for significant speedups
e8ce8af 
QT apps tend to call makeWrapper with a lot of arguments, which causes
noticable slowdowns (+100ms for app startup). The slow down boils down
to two reasons:

- the required string processing is O(N^2)
- bash is slow at doing the processing

By using the binary wrapper, we fix the second point, brining the
overhead down from 100ms to just 4ms or thereabouts.

I tested this change by rebuilding my whole system with it (I use
plasma). It booted and it works (that's where I type this commit
message), but I am not 100% sure if this won't break anything else.

Closes: NixOS#225871
@matklad matklad requested a review from ttuegel as a code owner April 12, 2023 11:19
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/plasma-emojier-is-very-slow-makeqtwrapper-is-accidentally-quadratic/27160/6

@SuperSandro2000 SuperSandro2000 requested a review from K900 April 12, 2023 11:20
SuperSandro2000
SuperSandro2000 approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be none breaking if all arguments are supported. Probably needs to go to staging.

@matklad
Copy link
Member Author

matklad commented Apr 12, 2023 •
edited
Loading

Can I change the base branch to staging in GitHub UI? Or is it better to close this PR and open the new one? This indeed should target staging, as it rebuilds every QT app.

EDIT: ah, that's explained in the manual, retargeted

@matklad matklad changed the base branch from master to staging April 12, 2023 11:38
@K900
Copy link
Contributor

K900 commented Apr 12, 2023

Should be fine, but I'd probably keep this for after 23.05 as it's the kind of change that has spooky implications.

@SuperSandro2000
Copy link
Member

Should be fine, but I'd probably keep this for after 23.05 as it's the kind of change that has spooky implications.

If all things build, this should just work but it could be that some things use flags not supported by the binary wrapper which would trigger a build failure. binary wrapper is already used by wrapGAppsHook and more things on darwin due to that the shebang must be a binary.

@K900
Copy link
Contributor

K900 commented Apr 12, 2023

I'm expecting it to be fine, but will it be "last staging cycle before release" fine? I'm less confident.

RaitoBezarius
RaitoBezarius approved these changes Apr 16, 2023
View reviewed changes
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try, but we should revert in case of large issues.

@RaitoBezarius RaitoBezarius merged commit 1a68995 into NixOS:staging Apr 16, 2023
@oxalica
Copy link
Contributor

oxalica commented Apr 16, 2023

Is there any plan to simplify the wrapper logic? Is it possible? It is not only slow but also as big as ~40KB for one bash wrapper (stat -L $(which dolphin)).

@SuperSandro2000
Copy link
Member

Do you mean the binary or the shell wrapper? The binary wrapper should already be a lot more optimized. The shell wrapper could probably be greatly optimized if we didn't check for every entry if the first and final : are there but since we want to deduplicate the entries it will probably stay longish. Maybe we could try to reduce the amount of entries added or symlinkJoin beforehand.

@matklad matklad deleted the makeBinaryWrapper branch April 19, 2023 11:25
@K900
Copy link
Contributor

K900 commented Apr 26, 2023

I think tdesktop is broken now?

❄ ❯ objdump -x =telegram-desktop
objdump: warning: /run/current-system/sw/bin/telegram-desktop has a section extending past end of file
objdump: /run/current-system/sw/bin/telegram-desktop: file format not recognized

@SuperSandro2000
Copy link
Member

I encountered the same

 ➜ strace telegram-desktop
execve("/etc/profiles/per-user/user/bin/telegram-desktop", ["telegram-desktop"], 0x7fff4612eaf0 /* 175 vars */) = 0
brk(NULL)             = 0x2143000
arch_prctl(0x3001 /* ARCH_??? */, 0x7ffe7105f100) = -1 EINVAL (Invalid argument)
--- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0x8} ---
+++ killed by SIGSEGV (core dumped) +++
Segmentation fault (core dumped)

@K900
Copy link
Contributor

K900 commented Apr 26, 2023

Never mind, tdesktop shoots itself in the foot: #228410

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/telegram-fails-to-run-sigsegv-address-boundary-error/27667/2

@pennae
Copy link
Contributor

pennae commented Apr 30, 2023

also kills gnuplot-qt:

make[3]: Nothing to be done for 'install-exec-am'.
 /nix/store/db6bfy1czsrfddhcb9xwcx9vb5aahv9c-coreutils-9.1/bin/mkdir -p '/nix/store/fmh89jbpd7kq9qqpn81m95qsh5wy7gw1-gnuplot-5.4.6/share/gnuplot/5.4/app-defaults'
 /nix/store/db6bfy1czsrfddhcb9xwcx9vb5aahv9c-coreutils-9.1/bin/mkdir -p '/nix/store/fmh89jbpd7kq9qqpn81m95qsh5wy7gw1-gnuplot-5.4.6/share/gnuplot/5.4'
 /nix/store/db6bfy1czsrfddhcb9xwcx9vb5aahv9c-coreutils-9.1/bin/install -c -m 644 Gnuplot '/nix/store/fmh89jbpd7kq9qqpn81m95qsh5wy7gw1-gnuplot-5.4.6/share/gnuplot/5.4/app-defaults'
 /nix/store/db6bfy1czsrfddhcb9xwcx9vb5aahv9c-coreutils-9.1/bin/install -c -m 644 colors_default.gp colors_podo.gp colors_mono.gp gnuplotrc '/nix/store/fmh89jbpd7kq9qqpn81m95qsh5wy7gw1-gnuplot-5.4.6/share/gnuplot/5.4'
make[3]: Leaving directory '/build/gnuplot-5.4.6/share'
make[2]: Leaving directory '/build/gnuplot-5.4.6/share'
make[1]: Leaving directory '/build/gnuplot-5.4.6/share'
make[1]: Entering directory '/build/gnuplot-5.4.6'
make[2]: Entering directory '/build/gnuplot-5.4.6'
make[2]: Nothing to be done for 'install-exec-am'.
make[2]: Nothing to be done for 'install-data-am'.
make[2]: Leaving directory '/build/gnuplot-5.4.6'
make[1]: Leaving directory '/build/gnuplot-5.4.6'
<stdin>: In function 'main':
<stdin>:25:6: error: #error makeCWrapper: Unknown argument --run
<stdin>:26:6: error: #error makeCWrapper: Unknown argument . /nix/store/rwgvww93dwp0dyzmhwjyd9p6nd95wiqh-set-gdfontpath-from-fontconfig.sh

This was referenced May 2, 2023
Closed
Closed
@wentasah
Copy link
Contributor

wentasah commented May 4, 2023

Fix for gnuplot_qt is in #229479. Can someone review it?

@K900
Copy link
Contributor

K900 commented May 4, 2023

A better fix is already in staging: #229154

@getchoo getchoo mentioned this pull request Sep 4, 2023
Closed
12 tasks
@SuperSamus SuperSamus mentioned this pull request Jan 20, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@RaitoBezarius RaitoBezarius RaitoBezarius approved these changes

@ttuegel ttuegel Awaiting requested review from ttuegel

@K900 K900 Awaiting requested review from K900

Assignees
No one assigned
Labels
6.topic: qt/kde 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

wrap-qt-apps-hooks leads to very slow wrapper scripts
8 participants
@matklad @nixos-discourse @K900 @SuperSandro2000 @oxalica @pennae @wentasah @RaitoBezarius