libtorrent icon indicating copy to clipboard operation
libtorrent copied to clipboard

Provide more flexible API

Open glassez opened this issue 1 year ago • 12 comments

I feel a big disadvantage of libtorrent is the fact that it does not allow many improvements to be implemented during the life cycle of the branch, as this may break ABI compatibility. I understand that it can hardly be completely eradicated, but it would be very useful to try to improve its flexibility as much as possible. One of the weaknesses is the use of "static" data structures in the interface, for example, torrent_status, peer_info. Maybe it would be better to replace them with hash tables? IIRC, then we'll be able to extend them without breaking ABI. Of course, it will cause a slight loss of performance, but it is unlikely that this will be noticeable enough, given that they are not used in critical sections of the code.

glassez avatar Mar 22 '23 09:03 glassez

The flexibility of the library can be increased by changing the code. It's open source and you can change it however you want. I did everything I needed in my version of the library a long time ago. That is what flexibility is all about - it can be done. It wouldn't be flexible if it couldn't be done on your own.

master255 avatar Mar 23 '23 20:03 master255

I did everything I needed in my version of the library a long time ago.

I don't use libtorrent only for personal purposes. The BitTorrent client I'm contributing to is supposed to be used with third-party libtorrent builds, so I can only rely on the original code. Thus if I need any changes, I have to try to introduce them into the mainstream.

glassez avatar Mar 24 '23 04:03 glassez

@glassez I don't think that's right. You need to create your libtorrent repository and add changes to it from the main branch. This is common practice when using an open source library. This way you need to update your version of the library less often than you do now, but instead you can make any changes to the library.

And the original library stays clean. At this point we need to fix bugs (I think they are still there), but not to reduce performance.

master255 avatar Mar 24 '23 18:03 master255

@glassez For example, my version of Libtorrent has long been able to transfer data over https, has support for editable torrents, swarm merging, streaming, works more correctly with trackers, has support for preloading files and several other improvements to simplify life. Right now I'm thinking about proxying traffic.

That's why I have the most advanced torrent client in the world right now (based on Libtorrent). And the functionality of your program was invented in 2005 and hasn't been improved since then. I think you should follow my path.

And my library is hard to update, but it's realistic to do it in a couple of days. That said, library updates don't bring as much new stuff as my edits do. In addition, new versions can have bugs of very high rank, which even Arvidn very difficult to fix.

It would be ideal to create a single, common repository for developing standard torrent programs with all the latest library improvements and as stable as possible. This is necessary because the latest version of the library is not completely stable and it is as conservative as possible. We need a stable and progressive version of the library.

I will update my library in 3 weeks. My library may well be a candidate for this version. I can share its code. To do this, message me on my Telegram.

master255 avatar Mar 25 '23 13:03 master255

@master255 It's not helpful to argue that this is not an issue. It clearly is. If you want to discuss how package managers and shared libraries are wrong and should work some other way to avoid requiring ABI compatibility, you could start a separate thread.

arvidn avatar Mar 25 '23 15:03 arvidn

@glassez I agree that the ABI should transition towards being more flexible from and ABI point of view.

Some examples of changes I've made in the past to support updates while preserving ABI are:

  1. settings_pack. Perhaps you remember the old days how there was a session_settings class, with fixed members. This made it impossible to introduce new settings without breaking ABI. The settings_pack is essentially a hash table of fields, but it still supports compact and efficient representation of the fields.
  2. alerts allows adding new alerts, but extending existing ones isn't so easy. Only leaf classes can be changed and only by adding new fields at the end. So, it's a little bit flexible.
  3. Using flag types with spare bits instead of bools. e.g. the torrent_flags_t that used to be individual bool fields.
  4. counters class replacing session_stats. The counters are stored in an array and indexed by dynamic indices, which allows adding more counters without affecting ABI. The old session_stats was just a class with all counters as fields.

Other examples (not implemented by libtorrent) of APIs that preserves ABI are:

  1. netlink sockets in linux. Where you send (binary) messages on a socket and receive (binary) messages back. This is a really crude and error prone interface though.
  2. using new functions and new structures. The win32 API has a lot of examples of old functions (e.g. GetFileSize) extended with a new function GetFileSizeEx().
  3. Classes that are used to pass information across the API boundary can have "dummy" fields just to reserve some space for future use. This was used by BeOS system classes. It's a bit tricky as it would really only work for trivially copyable data.

I'm generally a bit worried about going all the way to Objective C or Python level of dynamic data structure, like std::map<std::string, std::string>. I think an approach similar to counters and settings_pack would work for torrent_status as well.

arvidn avatar Mar 25 '23 15:03 arvidn

I think an approach similar to counters and settings_pack would work for torrent_status as well.

👍

glassez avatar Mar 25 '23 15:03 glassez

Really I care about issues like https://github.com/arvidn/libtorrent/issues/7346 which I consider as some kind of "bugs".

glassez avatar Mar 25 '23 15:03 glassez

@glassez I agree that the ABI should transition towards being more flexible from and ABI point of view.

Some examples of changes I've made in the past to support updates while preserving ABI are:

  1. settings_pack. Perhaps you remember the old days how there was a session_settings class, with fixed members. This made it impossible to introduce new settings without breaking ABI. The settings_pack is essentially a hash table of fields, but it still supports compact and efficient representation of the fields.
  2. alerts allows adding new alerts, but extending existing ones isn't so easy. Only leaf classes can be changed and only by adding new fields at the end. So, it's a little bit flexible.

@arvidn if the design indeed allows easily adding new alerts, could you please add the alert as I described to you in #7337 to trigger when the specified piece finishes downloading (without using set_piece_deadline) ? That would make it more efficient to handle streaming with regular torrenting.

SemiAccurate avatar Mar 26 '23 03:03 SemiAccurate

@SemiAccurate I don't think this feature trumps the pread disk I/O in priority. There is an alert for when pieces finish already, you can issue an explicit read_piece() when you get those. It will cost you one extra round-trip to the network thread, but it will work.

Otherwise, if you want to give this a stab yourself, you would need a new alert category, to avoid paying the cost for reading and posting piece data when it's not necessary. Alternatively, you could come up with a separate subscription scheme, similar to set_piece_deadline() that returns the piece or subscribes to it. The new alert is simple to add once the bulk of the feature is there. It would also need tests.

arvidn avatar Mar 26 '23 09:03 arvidn

@arvidn I don't know about pread disk I/O, I'm not a C/C++ programmer (like @AllSeeingEyeTolledEweSew I know Python.)

I'm not sure if you forgot the use case for this from #7337 but simply issuing an explicit read_piece() after piece_finished_alert() wouldn't work (it's not a matter of an extra round-trip to the network thread) because the latter alert doesn't trigger when the piece you want to read completes. The inefficiency is due to having to keep posting read_piece() and reposting (after failure) for the next specified piece until it downloads and thus succeeds.

Thus if you could add an alert say completed_piece_alert() that triggers when the specified piece completes downloading (or is already downloaded) then we don't have to keep polling for the next piece in the torrent to finish. I don't know about pread disk I/O, but this proposed alert would be very simple for you to add. Correct me if I'm wrong. Most of what it would do is already found in read_piece() except it would only trigger when the specified piece completes (or is already downloaded.)

SemiAccurate avatar Mar 26 '23 10:03 SemiAccurate

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Aug 12 '23 18:08 stale[bot]