Improving application architecture (Redesign).

[glassez]

Lately I've been increasingly involved in the development of qBittorrent. In this regard, I dive into the source code more and more. And the more I do it, the more I cultivate the desire to restore order there. qBittorrent needs overhaul - that is my conclusion. I will not give here the details, so as not to waste my time. Let me just say that there are a lot of problems here, starting with excessively long functions, various inconsistencies and cross-dependencies of different classes and subsystems, and to the lack of a clear architecture. One of the important problems is the widespread dependence of our code from libtorrent code. (Those who now consider me a fool and say "qBittorrent is based on libtorrent", I will answer immediately - no comments about this, if you do not understand what I mean - you can continue to regard me as a fool.) To @sledgehammer999 mostly: The circumstances are so that I can over the next few months to do this job. I would like to discuss issues of cooperation. This work is long and extensive code changes will be made. It is clear that I'm going to target it at a later release (after 3.2). As soon the lack of support for Qt4, I would prefer not to support Qt4. And still it is necessary to strictly set the minimum supported version of Qt5. For the same reasons (major changes) some time after the beginning of my work (a very short time, I think) I will not be able more to rebase my code. And this is a problem because there will be concurrent commits. Any ideas or comments?

Smaller issues. I also have the idea to use our modification of QtSingleApplication in all qBittorrent builds. I want to take as a basis QtSingleApplication from QtCreator, which is more powerful than the one that is now represented in QtSolutions (and which we use). Using libtorrent 0.16.x also questionable, but in any case, I initially will support both branches in order to better design the abstraction layer.

Next, I'll publish materials related to this (plans, ideas, models, and so on).

[sledgehammer999]

Let me just say that there are a lot of problems here, starting with excessively long functions, various inconsistencies and cross-dependencies of different classes and subsystems, and to the lack of a clear architecture.

I kinda agree on most of that.

One of the important problems is the widespread dependence of our code from libtorrent code.

I don't understand what you mean here. Can you give an example or explain more?

In the past few toys I am toying with the idea of dropping qt4 and libtorrent 0.16.x after v3.2.0. And I think I'll definitely do it.

Why different QtSingleApplication? We maintain only a Windows-specific modification and nothing else. What more features do we need from the one QtCreator uses?

As for how to continue work on this without stalling the rest of the development for you or having you constantly rebasing well I can think only one way. You should declare/acquire a "mutex" on files that you intend to work on for you next PR. You'll do that by opening a new issue stating what you are doing and which files you don't want touched and a rough ETA. Of course you should try to lock as few files as possible on each iteration. Or at least permit for some rebasing on your part too.

[glassez]

One of the important problems is the widespread dependence of our code from libtorrent code.

I don't understand what you mean here. Can you give an example or explain more?

We widely use the data types of libtorrent, call its functions, etc. It makes excessive dependence on it. Another problem is the set of States of the torrent, which gives us libtorrent, does not correspond to the set that we use on the user side. Hence, multiple transformations, check the version of libtorrent and so on. For example, in another version, the developers change something, and we have to edit a lot of code to insert there #ifdef's and so on. I want to isolate all interaction with libtorrent in some abstraction layer. I understand that to get rid of all problems completely will not work even with this approach, but will get rid of some.

[sledgehammer999]

Another problem is the set of States of the torrent, which gives us libtorrent, does not correspond to the set that we use on the user side.

Yes! I wanted to change that too.

Let's say that I agree with you on libtorrent but I am not too fond of over-abstracting stuff. I think libtorrent dependence should be the last thing to "fix". The effort should concentrate on the other stuff you mentioned first.

[glassez]

Let's say that I agree with you on libtorrent but I am not too fond of over-abstracting stuff. I think libtorrent dependence should be the last thing to "fix". The effort should concentrate on the other stuff you mentioned first.

Actually, I don't have to do it too Intrusive. I also don't like that. Much of what I mean by this abstraction will be made as a by-product of other work.

In the past few toys I am toying with the idea of dropping qt4 and libtorrent 0.16.x after v3.2.0. And I think I'll definitely do it.

For specifics. What do I expect? I, in principle, may not remove support for Qt4. But it will increase my work, and I will be a pity if this support will be removed when I finish.

[sledgehammer999]

For specifics. What do I expect? I, in principle, may not remove support for Qt4. But it will increase my work, and I will be a pity if this support will be removed when I finish.

One question. Do you use KDE at all? If yes, can you tell me what is the penetration of qt5 in relation to qt4? Are most apps using qt5 now?

[glassez]

Why different QtSingleApplication?

I thought to implement this functionality in our code more deeply, if possible. I don't like, for example, the current situation with daemon.

[glassez]

One question. Do you use KDE at all? If yes, can you tell me what is the penetration of qt5 in relation to qt4? Are most apps using qt5 now?

I am currently not related with Linux and KDE. I use them only for testing qBittorrent builds and even sometimes. As for Qt, then I guess it won't be me caring at first. So you have time to think it over.

[pmzqla]

One question. Do you use KDE at all? If yes, can you tell me what is the penetration of qt5 in relation to qt4? Are most apps using qt5 now?

If I'm not wrong the developement of KDE works as follows now. There are three different main components: Framework, Plasma and Applications. Currently KDE Frameworks 5 requires Qt5(.3?), Plasma 5 too if I'm not wrong, while I think most of the applications haven't drop Qt4 yet.

KDE SC 4.x is no longer developed (each 4.x release included all the components that are now released separately).

Here the schedule.

That saod, I'd expect a better Qt5 support by most of the distros in the near future.

[sledgehammer999]

Dropping 0.16.x is almost certain for the 3.3.x. Dropping qt4 is still considered.

[glassez]

OK. I will support yet.

[Gelmir]

Hello, just wanted to ask if anything will be done about downloadUrlAndSkipDialog and addMagnetSkipAddDlg in QBtSession. After implementing possibility of overriding global 'add paused' option for RSS feeds on per-rule basis we have quite an ugly set of default args, not counting headless ifdefs.

addTorrent and addMagnetUri might also need a revamp.

I propose using TorrentTempData or similar mechanics (maybe some kind of structure) for adding torrents and magnets, because with growing code complexity (and by complexity I mean different input conditionals for torrent/magnets addition) the number of arguments will grow by margin.

[glassez]

@Gelmir OK. I will bear in mind it.

[ximply]

That's a great job! Can some important data in ini file now turn to database, just like sqlite, because sometimes I found data lost when qBittorent crashed or exit with exceptions.

[chrishirst]

I dive into the source code more and more. And the more I do it, the more I cultivate the desire to restore order there. qBittorrent needs overhaul - that is my conclusion.

So it's not just me that finds it a disorganised mish-mash of almost incomprehensible 'bolt-ons' then.

[ngosang]

I would love to see some of these changes as soon as possible. I'm working on a new tab with upload and download speed charts. Let's make qBittorrent something bigger!

[sledgehammer999]

@glassez you mentioned that you are refactoring QBtSession and QTorrentHandle. You also mentioned that you won't have anything to show for about a month. However, I would be interested if you have time to write quickly what you are trying to achieve/do. Not very detailed or with code. I don't want an implementation of course. Just things like "I noticed that qtorrenthandle doesn't hold a state of the torrent and I refactoring it to always keep a cached copy of the last torrent_status". I want an idea where you are going because:

1. mostly about being informed
2. maybe I can suggest and alternative
3. to stop you from possibly wasting your time because I won't merge your changes. Highly unlikely. You have proven that you aren't stupid.

[glassez]

Just things like "I noticed that qtorrenthandle doesn't hold a state of the torrent and I refactoring it to always keep a cached copy of the last torrent_status".

@sledgehammer999 You read my mind! :+1: Discrete BitTorrent session state model is one of the first innovations. This is in part already exists in some places. Need to do this at a basic level.

However, I would be interested if you have time to write quickly what you are trying to achieve/do. Not very detailed or with code.

Well. I just didn't want to spend time on it. But you're right. Later I will provide a list of potential design problem (some even will have a way to correct them).

to stop you from possibly wasting your time because I won't merge your changes. Highly unlikely.

Will be very sorry if we will have conflicts on some issues. I'm always open to criticism and dialogue, if it is objectively.

[glassez]

You also mentioned that you won't have anything to show for about a month.

I may from time to time publish individual files (or fragments) until mergable/buildable PR is ready.

[sledgehammer999]

@sorokin since you did a lot of refactoring regarding cpu efficiency, your input here would be greatly appreciated. -after of course @glassez writes what he wants to do-

@glassez if new PRs arrive with trivial changes in QBtSession/QTorrentHandle I'll tag you to say if I can merge it.

[sledgehammer999]

@glassez One last thing: Try to keep it as simple as possible. No need for complex data structures or exotic design. -unless the situation can't be solved otherwise-. Are you also trying to tackle the issue of syncronously adding torrents? If yes, I and sorokin had an offtopic discussion about this with some pretty interesting ideas. I'll tell them to you if you are working on it too.

[glassez]

Are you also trying to tackle the issue of syncronously adding torrents? If yes, I and sorokin had an offtopic discussion about this with some pretty interesting ideas. I'll tell them to you if you are working on it too.

Yes. It is in my plans too. Point me to this discussion. And get some summary if possible.

[sledgehammer999]

I'll find it later. It was in a commit comment. The gist of it is: The big problem with the current loading of torrents on startup is that it slows down the qbt startup and especially the GUI. This is more prevalent when the user has many many torrents in his list. The problem arises because we synchronously load the torrent into the libtorrent session. This can be avoided if we use async_add_torrent() and listen for the appropriate alert. The only issue is that I want to finally implement #583. This is already implement in part by the custom values we load/save in the .fastresume files. However, we still need that ini file to determine the order of the torrents in the queue. Otherwise we would need to first read all the .fastresume fields, parse them, sort them and then load them. This isn't very efficient. The clever idea that @sorokin mentioned is: "We already code the hash in the filename. Why don't we code the queue number too. We'll only need to get a list of filenames and sort them.".

Of course we didn't talk about magnets. But this could be solved by having a bencoded file written in BT_backup called magnets. It would hold info about ALL the magnets that don't have metadata and of course their queue number. -even today the paused state of magnets isn't remembered-

So what were your plans on this?

[glassez]

Not so fast. I haven't delved into this issue. I just saw async_add_torrent() function and thought, "Why don't we use it? We must consider this option." So it is in my plans, but not in priority. First I would like to finish with a redesign.

[sledgehammer999]

First I would like to finish with a redesign.

No problem.

[glassez]

since @sorokin did a lot of refactoring regarding cpu efficiency...

I don't agree with some of his cases. Now I'm looking at QAlertDispatcher and it seems too far-fetched (I would even say - monstrous). No, all done well, if we consider it in itself, but I think I can make it much easier.

[glassez]

What were the reasons to refuse the use of pop_alerts() in favor of set_alert_dispatch()?

[sledgehammer999]

What were the reasons to refuse the use of pop_alerts() in favor of set_alert_dispatch()?

I think it showed as slow. More info in the #1668 megathread and in its PR #1703 Now I remembered the most important reason. We did away with the fixed timer. The dispatcher reacts in realtime. With pop_alerts() you react in fixed time. If you have many torrents in the queue, each time you pop_alerts() a lot of alerts will have accumulated and it will result in small gui freezes because you'll have to process them all at that time. I am not so sure we should change this. If the dispatcher can be simplified I am game. But IMO we should continue processing in realtime.

[sledgehammer999]

And the discussion about async_add_torrent() at the end of this commit https://github.com/qbittorrent/qBittorrent/commit/a1a5fb065e33caa7ec1e0d430b8413a652a11096

[glassez]

We did away with the fixed timer. The dispatcher reacts in realtime. With pop_alerts() you react in fixed time

Realtime is a Chimera. I have studied in detail the algorithm of the current code and came to the conclusion that we can get rid of QAlertDispatcher without any harm. Description: Now when the first alert appears (dispath() called) QAlertDispatcher adds an event to the Event Loop of the application and collects further alerts as long as the application does not handle this event (and process current alerts). And so on. While the app is idle, it processes the alerts one by one. When it is busy, the alerts have time to accumulate, and the app handles multiple. When using an idle timer (QTimer with interval = 0) so it turns out: QTimer adds an event to App Event Loop. When application can handle this event (his turn came), we call pop_alerts() and process current alerts. The rest is the same. While the app is idle, it processes the alerts one by one. When it is busy, the alerts have time to accumulate, and the app handles multiple. But only qBittorrent not doing it now, and libtorrent itself. I.e., we simplify the code by removing unnecessary duplication of existing functionality. The only thing here that may be like a slower work, this is the time when the application is idle and libtorrent is also idle. In this case, we still will occasionally call pop_alerts(), which will return no alerts. But the main thing here is that the app is IDLE, and we will not reduce its performance. With a strong desire we can make adaptive adjustment of the timer, so that to set some interval is greater than zero during libtorrent session idle. But it seems to me that the situation, when libtorrent session is idle, very rare, and it is possible not to worry about it.

[glassez]

With a strong desire we can make adaptive adjustment of the timer, so that to set some interval is greater than zero during libtorrent session idle. But it seems to me that the situation, when libtorrent session is idle, very rare, and it is possible not to worry about it.

I've tested it. Adaptive timer shows good results (without over-complicated code). I set the interval to 500 ms. after pop_alerts() returns no alert, and reset it to 0 when alerts appear. My initial assumption that it is possible to do without interval correction, was not correct. If you do not do this, it is high CPU usage when idle libtorrent session.

[sledgehammer999]

If you do not do this, it is high CPU usage when idle libtorrent session.

I suspected such a thing when I read your post late at night yesterday. Anyway, the decision should be based on objective data. What I intend to do when/if you do PR changing to a timer. I'll make 3 builds. One pre-QAlertDispatcher, one QAlertDispatcher and one with your timer. Test all 3 with 1000 torrents and run them under a performance analysis tool. Then compare the data. If the timer approach seems slightly worse than QAlertDispatcher, it will still be preferred due to its simplicity. If it shows too slow it will be dropped. @sorokin your expertise with vtune would be greatly appreciated. If you could do benchmarks that would be really great.

Sidenote: Today I'll be mostly unavailable. Why? My new pc parts arrive and I'll be building my new rig and reinstalling Windows(drivers, programs, etc) and transferring data from the old HDDs. Plus rebuilding my toolchain. Interesting specs for compiling: amd fx8320 (8 cores) and 8GB RAM.

[glassez]

If the timer approach seems slightly worse than QAlertDispatcher, it will still be preferred due to its simplicity. If it shows too slow it will be dropped.

Ok. I'll send PR soon. I have no way to test it with big data. So, if my assumptions regarding the performance will turn out to be wrong, you can throw it without regret. Besides, I have another idea about this. Some intermediate option.

[sledgehammer999]

It took me way longer than I thought to have things working. I haven't managed yet to completely transfer old settings/files/data over to the new system. The good news now are these

• I am using Windows 7 now
• I am also using msvc 2013(express).
• I finally managed to make it compile qbt that will also run on Windows XP :D
• Now that I use msvc2013, I'll probably use it permanently for the official releases. That means that we could start using c++11. But this isn't panacea...
• Way faster builds. My old rig build v3.1.11 in 27-30mins. The new rig builds the same version(with same accompanying libs) in ~5:25mins :smile:

From tomorrow I'll slowly begin the reviewing all the PRs that have accumulated.

[glassez]

That means that we could start using c++11.

I love it! But if you really want to do this, you must formally specify the minimum version of the compilers that qBittorrent supports. Otherwise we will get a new headache, and not improving.

[glassez]

qBittorrent top-level design (To-Be Model):

Here is the design that I want to embody. Its key components:

1. Application. It owns all other components, creates and destroys them, exposes one to other components (if possible). Also implements the basic CLI.
2. Core. It owns libtorrent and other core-wide components and utils (like BitTorrent Session, Tracker etc.).
3. GUI. GUI components. It can uses Core components (trough Application).
4. WebUI. Web access implementation. It can uses Core components (trough Application).

The most important thing here is that we need to get rid of cross-dependencies wherever it is not required (and such cases are very rare). Core should not in any way depend on UI components. UI components use Core, and not Vice versa. It's not supposed to know about that and how its uses, but only to provide an interface in terms of their subject area. This mainly applies to BitTorrent session, but other Core code should not have external dependencies. Ideally, the Core should compile no matter what other top-level components used in this build, while not having any #ifndef DISABLE_GUI etc.

One more thing. I want the application structure was reflected everywhere - in the directory structure of the project, and used the names (namespaces, where necessary), and the method of inclusion of headers. Opening any project file, we need to understand that it uses, what classes and types belong to one or another part of the application structure.

[Gelmir]

@glassez, where would RSS/Search code go in this model?

[glassez]

I haven't come to RSS... as for the search, its basic components must belong to the Core that you can use them not only from the GUI. Already have a request to access them from WebUI.

[glassez]

@sledgehammer999 There is really a lot of work, and I can't do everything at once. So we'll have to do it by parts. The first part is torrents data caching. Also I can implement asynchronous torrents addition, if we make a decision regarding the queue storage. I have read your discussion. The option of encoding priority in the name of .fastresume file is simple only at first glance. It's just when loading, but can be difficult when you save, because you need to worry about deleting old files, which names contains outdated priority. Although, we can keep the current name of .fastresume file, and if the new will not match with it, delete the old file. Other options here are either to store an ordered list of torrents in a separate file (you can use a simple text file for quick parsing), or to use a database (we could store queue and other data there, including fastresume and persistent data).

[sledgehammer999]

@glassez I generally agree with the picture.

about the async loading: Isn't it simpler to manage the deleting of old files this way. Fist of all, the queue number should be coded in the fastresume filename in this way <hash>.<queue number>.fastresume. Then before saving the new fastresume, we query the file list in BT_backup, filter by .fastresume and delete all those that begin with our hash. No need to remember the old queue number.

[glassez]

Storing the queue number (and not the full file name, as I suggested earlier), we will win in performance, since get rid of unnecessary disk operations and sorts.

[glassez]

Sorry, not sorts but filtering.

[sledgehammer999]

Storing the queue number (and not the full file name, as I suggested earlier), we will win in performance, since get rid of unnecessary disk operations and sorts.

Oh you were talking only about the storing of the queue number in a txt/db On second thought, do we really need to store it on disk? Why not have a QHash that will store the previous file name for each hash? We look it up before saving the new fastresume, we save it, we delete the old one, we update the QHash. I don't think this needs to survive between sessions.

[glassez]

Blame the language barrier! I do not propose to keep the queue in the database. I just suggested several cases:

1. We do not include queue number into file name and store an ordered list of torrents (its hashes) in a separate file (in plain text file as an option). During session startup we just read this file and obtain the order of torrents and then load approprite .fastresume files.
2. We include queue number into file name, we also store last queue number of torrents (or even last filename) in memory and delete old files.
3. We do not use .fastresume file at all, and the fastresume data stored in the database. There we store the queue number and other persistent data. During session startup we just query records from database ordered by queue number.

[sledgehammer999]

Number 2 is the better of the 3.

[glassez]

Number 2 is the better of the 3.

I already understood that you dislike the database. But now it does not matter. I'm going to implement the second option. Perhaps some other time I implement this using a database as an alternative (possibly specified in the configuration). But not now. Now there are more important things to do.

Last clarification. Now all persistent data will be stored in .fastresume file? Queue number was the last that was preventing the rejection from the storing this data in a separate file?

[sledgehammer999]

Queue number was the last that was preventing the rejection from the storing this data in a separate file?

IIRC, yes. Unless we added some extra field in the meantime. Anyway, if there is some obstacle you will discover it during the refactoring. But I am confident that there isn't any.

[sledgehammer999]

@glassez I think all your next changes will be for v3.3.0. I also am considering dropping qt4 entirely after v3.2.0. Do you have know any reason why I shouldn't do this? @pmzqla IIRC you use linux right? qt5 support is ok there, right? We don't have any problems? @Noctem what is qt5 support in OSX?

@glassez if qt4 is dropped we can choose to use QSaveFile and be sure no data is lost. (and of course it will be used for the .inis too)

[pmzqla]

I use Debian (sid). I've just built qBittorrent with Qt5 with no problems. This means that it will be possible to build qBittorrent with Qt5 on the next Debian stable.

EDIT: and it seems lot of stuff had been backported, so it's maybe even possible to build it with Qt5 on the current stable.

[Noctem]

@sledgehammer999 Qt5 is working well in OS X. I use 3.2.0 with Qt5 everyday, and it looks better than Qt4. Do you plan on distributing the 3.2 release with Qt5?

[glassez]

I think all your next changes will be for v3.3.0. I also am considering dropping qt4 entirely after v3.2.0. Do you have know any reason why I shouldn't do this?

The only reason that may prevent migration to Qt5 is possible lack of support for it in linux distros. You need to check what version of Qt are available in the main distros.