trunk moves from intrusive_ptr to shared_ptr

[GoogleCodeExporter]

I tried to compile qbittorrent with libtorrent from trunk. The compilation 
fails. The problem is that qbittorrent uses 
boost::intrusive_ptr<libtorrent::torrent_info> and the revision r10084 "land 
libtorrent_aio branch in trunk" remove inheritance of torrent_info from 
intrusive_ptr_base<torrent_info>.

I don't know if this breakage is intentional. Should I update a code of 
qbittorrent to not use boost::intrusive_ptr<libtorrent::torrent_info>? Or 
should libtorrent be updated to support old behavior?

In file included from /home/ivan/d/qbittorrent/src/addnewtorrentdialog.cpp:31:
In file included from /home/ivan/d/qbittorrent/src/addnewtorrentdialog.h:39:
In file included from 
/home/ivan/d/qbittorrent/src/qtlibtorrent/qtorrenthandle.h:35:
In file included from /usr/local/include/libtorrent/torrent_handle.hpp:58:
In file included from /usr/local/include/libtorrent/storage.hpp:49:
In file included from /usr/include/boost/intrusive_ptr.hpp:16:
/usr/include/boost/smart_ptr/intrusive_ptr.hpp:97:23: error: use of undeclared 
identifier 'intrusive_ptr_release'
        if( px != 0 ) intrusive_ptr_release( px );
                      ^
/home/ivan/d/qbittorrent/src/addnewtorrentdialog.cpp:53:22: note: in 
instantiation of member function 
'boost::intrusive_ptr<libtorrent::torrent_info>::~intrusive_ptr' requested
      here
AddNewTorrentDialog::AddNewTorrentDialog(QWidget *parent) :
                     ^

Original issue reported on code.google.com by vanya...@gmail.com on 12 Oct 2014 at 11:26

[GoogleCodeExporter]

This was a deliberate breaking change. The rationale being to move closer to 
standard types (i.e. std::shared_ptr). With make_shared() the cost of a 
shared_ptr is not much higher than intrusive_ptr. I'm willing to be convinced 
it's a bad idea though.

One thing I did not realize immediately is that shared_ptr is twice as big as 
intrusive_ptr. This limits the places they can be used efficiently internally 
at least.

Original comment by arvid.no...@gmail.com on 12 Oct 2014 at 4:57

• Changed title: trunk moves from intrusive_ptr to shared_ptr

[GoogleCodeExporter]

> This was a deliberate breaking change.

Ok. Then I update qbittorrent.

> I'm willing to be convinced it's a bad idea though.

Could you consider to evaluate std::unique_ptr from C++11? Quick profiling of 
libtorrent thread reveals that the forth most expensive function used in 
libtorrent is atomic_exchange_and_add used in intrusive_ptr. On my snapshot 
libtorrent thread spends 5.5% of time incrementing counters of intrusive_ptr.

https://cloud.githubusercontent.com/assets/286877/4607562/8d27380e-5254-11e4-967
e-eb66bcc47891.png

Look at line 4. Code in tracker_manager::incoming_packet() looks like linear 
search over list of tracker_connections. Could we refactor the code to use a 
map/unordered_map?

Currently tracker_manager::incoming_packet() takes 10.5% of time of libtorrent 
thread:

https://cloud.githubusercontent.com/assets/286877/4607577/4f1df9c0-5255-11e4-98d
d-5a105f772e4f.png

Original comment by vanya...@gmail.com on 12 Oct 2014 at 9:19

[GoogleCodeExporter]

I reread my comment number 2. I think it is not clear enough.

My suggestions are:

1. Consider using std::unique_ptr to eliminate cost of reference counting.
2. Optimize tracker_manager::incoming_packet() to use map/unordered_map instead 
of linear search.

Original comment by vanya...@gmail.com on 12 Oct 2014 at 9:42

[GoogleCodeExporter]

Hi. I've almost compiled qbittorrent with trunk libtorrent. Here are the list 
of changes that affect qbittorrent (just check that it is intentional):

1. no intrusive_ptr_base for torrent_info.
2. session_settings::outgoing_ports -> {outgoing_port, num_outgoing_ports}
3. cache_status::job_queue_length missing -- I don't know what should I use 
instead
4. listen_failed_alert::endpoint missing -- But it is still there in 
listen_succeeded_alert, it is ok?

Also I have some problem with linking. In libtorrent-1.0.2.tar there was 
configure script. In trunk it is absent. So I had to use CMake to build 
libtorrent. I had to fix CMakeLists.txt in order to resolve some linking 
errors: http://pastebin.com/BPU18uVx

CMakeLists misses files crc32c, resolver and dos_blocker. Also it doesn't link 
with boost_random. A complete diff: http://pastebin.com/tSEGk5xY

Also I found that qbittorrent links with two TORRENT_EXTRA_EXPORT functions: 
base32decode and has_parent_path. As I understand these functions are only for 
internal use in tests. As I understand the problem is that distributions builds 
qbittorrent with tests enabled, so these functions got to .so. Is it OK for 
qbittorrent to continue to use these functions?

Original comment by vanya...@gmail.com on 14 Oct 2014 at 8:08

[GoogleCodeExporter]

unique_ptr<> solves a different problem than shared_ptr. Also, I'm not entirely 
sure I'm ready to switch to c++11 yet. I'd have to ask around a bit if people 
are OK with that first.

Thanks for pointing out the performance issue with udp trackers though. I've 
checked in a half-assed fix to RC_1_0 and trunk (which might provide the bulk 
of the improvements) and I intend to fix it properly in trunk soon.

In response to your bullet points:

(1) By this you mean that you have to use shared_ptr instead of intrusive_ptr, 
right? As far as I can tell, there's no other aspect of this change.

(3) I've deprecated a bunch of fields in the cache_status structure (I must 
have missed that I removed this one, and that it's a regression). I've been 
moving stats over to use the performance_counter api. i.e. 
session::post_session_stats() and the session_stats_alert. It's a more flexible 
and extensible stats mechanism. It can be extended without breaking backwards 
ABI compatibility. As for this specific gauge, it's now called 
counters::queued_disk_jobs.

(4) the endpoint was replaced with a string called "interface". This is a 
generalization. It contains the same information as the endpoint, but now it's 
possible to bind to a device name as well, which cannot be represented by an 
endpoint. For normal endpoints though, it's just the string representation of 
it.

Original comment by arvid.no...@gmail.com on 15 Oct 2014 at 2:36

[GoogleCodeExporter]

thanks for the cmake patch. I've applied it to trunk.
./configure is generated and is only included in tarballs. To generate it 
yourself, run:

./autotool.sh

regarding TORRENT_EXTRA_EXPORT, that is intended just for the tests (mostly to 
be able to test internal components). Perhaps the fact that my build_dist.sh 
script builds and runs the tests makes the default configure script include 
this build flag as well. Ideally these symbols would not be exported and not 
relied upon by clients.

Now, I haven't converted all my examples to avoid such dependencies either yet, 
and I'm willing to be convinced that some symbols maybe should be exported as 
part of the official API after all.

Original comment by arvid.no...@gmail.com on 15 Oct 2014 at 3:05

[GoogleCodeExporter]

I think I have a working patch for the UDP trackers to be more efficient. Would 
you mind testing it?
It's against trunk @ 10399.

http://dpaste.com/3QR50JW

Original comment by arvid.no...@gmail.com on 20 Oct 2014 at 3:47

[GoogleCodeExporter]

I will consider this fixed now, since I checked in the udp tracker optimization 
into trunk. Please let me know if there's something else relevant in here that 
I missed.

Original comment by arvid.no...@gmail.com on 21 Oct 2014 at 12:36

• Changed state: Fixed

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

> I've checked in a half-assed fix to RC_1_0 and trunk

Your fix where you changed the order of handlers I tested it. And I haven't 
seen tracker_manager::incoming_packet() to took much time. So this workaround 
works. Thanks.

> I think I have a working patch for the UDP trackers to be more efficient. 
Would you mind testing it?

I will test it. Thanks.

Original comment by vanya...@gmail.com on 22 Oct 2014 at 11:09