Port to Qt6

[eraxillan]

Qt5 open source edition lifetime is over, all updates will be available only to commercial edition users.
So any open source Qt program should be ported to Qt6 as soon as possible, due to possible vulnerabilities risks at least.
It's a Qt vendor company position AFAIK.

So I've tried to do port your code myself, and managed to fix all compilation issues.
However, got segmentation fault while trying to open a repository, in a GitCache class method.
I suspect some kind of thread sync issue, but still cannot figure it.
(For Qt6, one must use QRecursiveMutex instead of QMutex + recursive flag, although this don't like a problem at first glance) That's my code, where all monkey job was already done: https://github.com/eraxillan/GitQlient/tree/dev/qt6_port

Of course, you can just decide to stick with Qt5.
Qt6 isn't ready for production yet: it still haven't WebEngine modules you used, for example. However i still ask you (and other users) to help me with porting to Qt6 due to lack of further support of Qt5.
It's a one-time job i suppose.

P.S. My fixes do not touch Qt5 code, it still work as expected.

[francescmm]

Hi @eraxillan,

I've already tried to run Qt6 but as you said, there are some dependencies that are not yet ported in Qt code itself. The plan is to move to Qt6 once a production and stable release it's done. I think that will happen sometime around the release of Qt 6.3 or 6.4. Then plan is to migrate to CMake as well.

However, until Qt6 releases perform as good as they do in Qt5 and linux systems start using Qt6 I'll wait for it. There is no real benefit of using Qt6 right now and the threading would require a whole rewriting. Right now it's better I focus on refactoring and modularizing the code of GitQlient so when the port to Qt6 is available it goes smoothly.

[eraxillan]

@francescmm Ok, thank you for fast respond and good luck with your project!
Still hope my code will be useful for you some day.

Also i suppose to create a new issue for threading model refactoring discussion: e.g. maybe better to use QtConcurrent or other high-level stuff?

[francescmm]

Well, if you implement the port Im really happy to take it! That would be really awesome.

What I meant is that I don't have the time right now to move on with these changes, but for sure I can help in the future.

[eraxillan]

Ok, if i will manage resolve issue with sigsegv using GitCache::mCommits field, and probably another issues found, i'll create pull request.
Looks like memory corruption, so i try at least isolate the problem first.

[fizzyade]

CMake is a much higher priority than Qt6 imho.

Qt6 is alpha at best, it's not going to have feature parity with 5 for quite a while, and although 5 is now behind a paywall, 5.15.2 is pretty solid, feature-rich and is going to continue to work perfectly for many years to come.

I'd be wary of doing anything in Qt6 at the moment because they're bound to change things from release to release and chasing your own issues can be hard enough, let alone having the rug pulled from under your feet at the same time.

[francescmm]

Totally agree with you, @fizzyade ! The plan is to start working on the CMake soon. I'd like to include it for the 1.3.1 or even 1.3.2.

[eraxillan]

@fizzyade nice choice: unsupported Qt5 and alpha Qt6, heh.
Just note: one should keep in mind absence of security issues fixes.
Paranoid man should launch Qt5 apps in some kind of sandbox: firejail
Maybe it worth mentioning in documentation.
Also author can explicitly note in README that Qt6 is not supported yet.

[fizzyade]

Totally agree with you, @fizzyade ! The plan is to start working on the CMake soon. I'd like to include it for the 1.3.1 or even 1.3.2.

CMake is awesome. Sure it has idiosyncrasies, but it's so much more powerful than QMake. In Pingnoo I basically created a meta-language for CMake using macros, this drastically reduces the amount of stuff I have to put in CMakeLists.txt and means that there's consistency from plugin to plugin (or components as I call them).

It can be pretty daunting moving an existing project to CMake, but once you start realising just how flexible it is and how you can make use of dynamic things in the language you wonder why you didn't use it earlier.

[fizzyade]

@fizzyade nice choice: unsupported Qt5 and alpha Qt6, heh. Just note: one should keep in mind absence of security issues fixes. Paranoid man should launch Qt5 apps in some kind of sandbox: firejail Maybe it worth mentioning in documentation. Also author can explicitly note in README that Qt6 is not supported yet.

That's what we would call "Hobsons Choice" here in the UK!

I can't imagine there are many (any?) potential issues from a security issue in Qt, you're not (or shouldn't be!) running GitQlient under a privileged account. Heck I'd wager there's probably still a lot of people shipping and developing applications using Qt 4!

The Qt 5-> Qt 6 migration probably isn't worth worrying about until KDE switches to using 6.

The paywall for 5 sucks though, I think it's incredibly short-sighted of the Qt company and I'm hoping that there's a huge drop-off of user contributions and fixes that will force them into a rethink of their strategy.

[francescmm]

@eraxillan paranoid man should definetely not run any application within a privileged account as @fizzyade said.

I'm fine if people in their spear time want to try to upgrade to Qt6. Officially I won't support it until the versions I've already said (6.3 or 6.4). Moreover when non of the big apps or frameworks (KDE) are not doing it in a near future.

About stating that in the README, I could add it sure but I think that whoever looks at the front page of the repo will see that the current compiling version is 5.15. But remember that is not that GitQlient doesn't support Qt6, is that Qt6 has changed so much that is Qt who doesn't support GitQlient (joking :yum: ).

I don't like how they implemented the paywall but I also understand that they are a company that need to make some profits and feed their employees. I ask to pleas avoid this topic since it's not related with GitQlient and the issue description.

[eraxillan]

@francescmm sorry for flood in this issue.
Good news: i've managed to run GitQlient on Qt 6.1.0! :)
I am a bad tester, and just checked that repo loaded correctly.

The problem was in GitCache::mCommits field.
You fill it with pointers to hashmap GitCache::mCommitsMap, but looks like in Qt6 pointers to QHash items are not valid all program lifetime anymore.
Then you modify QHash, at some time it will rebuild it's internal memory structure and "old" pointers to it's items become invalid ones.
It's just a theory though.

So i just replaced pointers to stack-allocated value that way: QVector<CommitInfo> mCommits;,
and rewrote client code with corresponding way.
Of course this cause extra copies :(
However CommitInfo class contains only Qt-based fields and so support copy-on-write idiom.

[francescmm]

@eraxillan The problem is that in this case the COW paradigm wouldn't work because we're not talking about a single object but about containers. In this case the containers do a deep copy of the object.

This is not only a problem of more memory being filled, it's also about the fact that you potentially lose the data syncronization. Every time you update a CommitInfo object in one container, you will have to do the same in all the other containers. So the computational cost also increases.

I'd suggest you to check for a different approach on this.

[eraxillan]

@francescmm i found another way to fix the memory issue, it's simple, but requires extra loop:

Code

``` void GitCache::setup(const WipRevisionInfo &wipInfo, const QList &commits) { ... for (const auto &commit : commits) { if (commit.isValid()) { insertCommitInfo(commit, count); ++count; } } // Now container is ready and will not change at future, so now // one can ref to it's items mCommits[0] = &mCommitsMap[CommitInfo::ZERO_SHA]; count = 1; for (const auto &commit : commits) { if (commit.isValid()) { mCommits.replace(count, &mCommitsMap[commit.sha()]); ++count; } } } void GitCache::insertCommitInfo(CommitInfo rev, int orderIdx) { ... mCommitsMap[sha] = rev; // Do not ref to container items until it's modification is over! // mCommits.replace(orderIdx, &mCommitsMap[sha]); } ```

In Qt6 we can reference to internal representation of QHash item only if it is read-only.
Any modification operation can invalidate internal memory structure of QHash.
So one can not at one time modify that kind of container and refer to it's items.
That's why i just split one loop to two.

What's yours opinion: is such kind of fix is acceptable?
I have no other ideas yet, at least without heavy refactoring of data structures in existing code.

[francescmm]

I'm not convinced at all about having more loops, instead I would evaluate the cost of using QMap. I think that there are some improvements I could do to the cache before Qt 6 goes in.

The problem here is that this operation is done when the graph needs a "deep" refresh. And with deep I mean everytime the cache needs a refresh (e.g. when checking out a different branch).

Do you have a branch where I could check all the changes?

[eraxillan]

Well, i'm really curious whether just single container can be used instead of several ones.
Also one can get rid of mutex using something like this As far as i can understand, you introduce several containers to optimize search?
Hope that didn't sound offensive, i just want to understand your code.

My Qt6 port branch is here: https://github.com/eraxillan/GitQlient/tree/dev/qt6_port

[francescmm]

@eraxillan, it's good you ask, so don't worry because it's not offensive at all.

As you already deducted I use two containers for optimization in searches. In 90% of the cases, searching by commit SHA it's enought the problem appears when trying to use QTableView/QListView/QTreeView. This type of views in Qt search by row/column, that means that somehow I need to provide the information not by commit SHA, but by row number.

That row number is directly related with the graph representation (by git). I could store the ID in the commit but I would have to keep some kind of linked list inside the CommitInfo class to it's parent and that information is because of Qt, not related with Git representation of a commit. In addition that would cause a really painful lookup to get the information anyway.

If you have an alternative I'd be really happy to hear, because I couldn't think in a simplier way than keeping a real container (the QHash) and a vector filled with raw pointers... :disappointed:

I'll check the branch during the weekend, thanks!

[eraxillan]

@francescmm i found alternative solution: use one ordered map instead of unordered one plus vector.
There are many solution, i've tested this one: link
It behave like Java's LinkedHashMap class.
If the key order is always the same, when one can find the "row index" for just using std::distance from map beginning and required item.

I see two possible problems in this solution:

1. Possible performance issues: need further investigation
2. New third-party dependency e.g. as git submodule (it's a header-only lib)

And what do you think?
Still not commited experimental code: it will not compile without external library.
But at first glance it works correctly.

[francescmm]

Hi @eraxillan I've checked the branch and I'm happy to see that there are not that many changes needed to be done. However, until Qt6 is ready I cannot see the how useful could be to add support for it. I plan to split the GitServer part in a library/plugin so probably that will make it easier for you.

About the cache thing. I've checked and the problem of std::distance is that the complexity is linear, it doesn't solve the loop problem, just makes it easy to ready and code. I still think that it's better to have two containers. I would have to check what's the performance of QHash vs QMap, because for big repositories without commit limit that can hurt.

[eraxillan]

@francescmm About Qt6 port: BTW Qt6-specific code was wrapped by ifdef, so it should not break existing Qt5 code.
Feel free to use it when you decide Qt6 is worth to try.

About cache: if you got some time/interest, please check the ordered_map version of code (GitCache.cpp, GitCache.h).
std::distance used only in GitCache::getCommitPos function, and probably it can be rewritten in more optimal way.

[francescmm]

I checked the code and the std::distance wouldn't be that much of a problem compared to the QVector::indexOf. They both do a linear search.

I've been digging into the Qt release map and I assume that the plans for a 6.3 or 6.4 are April/September 2022. Right now 5.15 is not even shipped in Ubuntu 20.04 LTS and considering that 16.04 is still alive until the end of the month I would say that there is still a long way until Qt 6 is widely adopted.

I know that you wrapped the code with ifdefs but not even the current behavior is adaptable (all the webkit would be gone and I still made my mind about whether I want to keep it or not). The first thing I want to do is to get rid of the current ifdefs before I put some more.

So, after thinking about this I've decided to close the ticket. I cannot see any benefit for doing it right now and the certainty that Qt will introduce big breaking changes between minors doesn't help.