Page MenuHome GnuPG

Kleopatra, GPG: AboutData ctor on Windows takes too long
Closed, ResolvedPublic

Description

With the code:

qCDebug(KLEOPATRA_LOG) << "Startup timing:" << timer.elapsed() << "ms elapsed: Service created";

AboutData aboutData;
KAboutData::setApplicationData(aboutData);
/* This is more expensive as it sounds as it might run a verification
 * on a signed Version file and initializes the whole GpgME::Engine. */
qCDebug(KLEOPATRA_LOG) << "Startup timing:" << timer.elapsed() << "ms elapsed: About data created";

I see:

[13096] org.kde.pim.kleopatra: Startup timing: 284 ms elapsed: Service created
[13096] org.kde.pim.libkleo: No signed VERSION file found.
[13096] org.kde.pim.libkleo: Parsed 2.2.41 as:  2 . 2 . 41 .
[13096] org.kde.pim.libkleo: Running gpgconf --show-versions ...
[13096] org.kde.pim.libkleo: Running gpgconf --show-versions timed out after 1 second.
[13096] QProcess: Destroyed while process ("C:\\Gpg4win\\bin\\..\\..\\GnuPG\\bin\\gpgconf.exe") is still running.
[13096] org.kde.pim.kleopatra: Startup timing: 1815 ms elapsed: About data created

So what we see here is that gpgconf --show-versions takes more then a second and is even timed out. Afaik this is because it in turn calls all the processes which then do their initialization. Kleopatra caches this later but this is the first call.

On the Kleopatra side I think the solution will be to put the very important parts of KAboutData like Application Name and Organization which is used for config files and just put "Loading..." in the information of the branding texts. I guess they will never be opened before they are checked. Then we can put that into a background thread. As in the case of a signed version file the about data init would also do a gpgv call with a version validation. The timing shown above is without a signed VERSION file.

On the gpgconf side. I don't really know, its kind of a design issue that gpgconf starts each process to query information about the process so that all gpgconf calls take a while. We should try to delay this as long as possible and show UI before this.

This is a bit related to T6331 where a problem is that the KUniqueService is initialized too late on Windows to avoid parallel calls.

Event Timeline

aheinecke created this task.
aheinecke lowered the priority of this task from High to Normal.Jan 11 2023, 12:10 PM

by moving the KUniqueService before this and with the change b58cf129f the priority is reduced. It will still take 200ms so we might want to do something about this but it is not prio high as the 200ms are only on first run.

I would like to take this on myself by creating a gpgversioninfo class which will have signal / slot based API for both the SWDB Query and the version checks, both currently delay the startup too much.

aheinecke mentioned this in Unknown Object (Event).Aug 28 2023, 7:20 AM

I have a bad patch for this. D567 But I think we need a better solution that works without libkleo and could live in KCoreAddons as a change to KAboutData so that we can reuse it better for T6594

aheinecke mentioned this in Unknown Object (Event).Sep 25 2023, 8:52 AM
aheinecke mentioned this in Unknown Object (Event).Oct 16 2023, 8:58 AM
aheinecke changed the task status from Open to Testing.Nov 15 2023, 9:34 AM
aheinecke added a project: vsd32.

This is in vsd32. But I am not sure what to test here. You could take a previous beta and look at the startup timining debug output which says "mainwindow shown" and compare that to beta-277? The mainwindow shown timing debug output is not part of 3.1.26

aheinecke changed the task status from Testing to Open.Nov 15 2023, 11:17 AM
aheinecke moved this task from QA to WiP on the vsd32 board.

Welcomewidget is broken now: Welcome to Kleopatra: [Kleopatra] instead of showing the version.

The reason for this is that this still uses the libkleo::gpg4win class for the version info, the about data in GpgOLs help dialog should be similarly broken.

I know that there is an issue here that the about data in the option dialog of gpgol that is fixed with afb6ce9ce00538242ac69434f586749217f9f619 but did not make it in the current beta. And since this is also related to the tender version i leave this a bit open for now. I think we also might need to reload the welcomewidget in case the signature verification takes longer then constructing the welcomewidget.

So this is weird to me to describe. After merging: https://invent.kde.org/pim/kleopatra/-/merge_requests/72 It would no longer use the updated KAboutData from updateAboutDataFromSettings in the loadBackendVersions code. There it would see the original aboutdata and then set that with the added otherText.

So for now I reverted https://invent.kde.org/pim/kleopatra/-/merge_requests/72 in the Gpg4win repo. And I am seriously thinking of just dropping this whole Asyncness of the about data altogether, and live with the load time. But I do not understand at all why the code from https://invent.kde.org/pim/kleopatra/-/merge_requests/72 does not work.

@svuorela do you have an idea?

updateAboutDataFromSettings is calling the static KAboutData::applicationData() and KAboutData::setApplicationData() from the c'tor of AboutData, i.e. before the AboutData has been fully constructed and has actually been set by the setApplicationData() call in main(). And then the call in main() overwrites the application data set by updateAboutDataFromSettings. Before the change it worked because the thread called updateAboutDataFromSettings after the application data had been set in main().

You have to pass this (of AboutData) to all those local static functions. Or, better, make all those static functions members of AboutData.

Nevermind, I realize the problem since we are in the constructor KAboutData::setApplicationData is set when the constructor returns. So it does not makes sense for us to already call KAboutData::applicationData in this constructor and update it in the constructor only to have it overwritten when the function returns. Wit the constructed KAboutData object. Doing it in a thread works because then the thread is run after the constructor initially set the the application Data.

Should be simple enough to properly fix this by doing the load from settings / verification inside of the constructor.

Sorry, I didn't really pay enough attention when I reviewed this, but I thought you had tested this.

Ah sorry i did not read your two messages that analyzed this correctly before I wrote my comment I had not refreshed the page.

I thought I had tested this, too. But apparently I lost track of my versions and did not.

It is now: https://invent.kde.org/pim/kleopatra/-/merge_requests/75
I realized that for Qt6 the include qtextcodec and setIniCodec must be removed. I should probably ifdef them to make merging more painless.

This now works to my liking.

ebo edited projects, added vsd32 (vsd-3.2.0); removed vsd32.