Opened 11 years ago

Closed 9 years ago

#2808 closed Enhancement (invalid)

Threading infrastructure

Reported by: jch Owned by: charles
Priority: Normal Milestone: None Set
Component: libtransmission Version: 1.82
Severity: Normal Keywords:
Cc:

Description

This is something that has been lying in my repository for ages. I've just rebased it against the current trunk, and I'm submitting it for Charles' consideration.

This imports the threadpool infrastructure used in the CPC runtime (used by Hekate) into Transmission. Right now, it does nothing interesting, except replacing runInEventThread with a wrapper against the threadpool equivalent (which will be somewhat more efficient).

I'm planning to use this code to move chunk verification and libcurl interactions into the threadpool -- in principle, this might solve a number of the latency issues that we have.

Charles, while the threadpool code itself has had a reasonable amount of testing as part of CPC, the patch against libtransmission has not. Careful eyeballing of the libtransmission bits is required.

Change History (15)

Changed 11 years ago by jch

Changed 11 years ago by jch

comment:1 Changed 11 years ago by charles

jch: the code looks well-written, but does it makes sense for libtransmission? The DNS lookups are done in a nonblocking manner, which is faster than having them block in another thread. And you only want to do one hash check at a time, so only one worker thread is needed for that.

comment:2 follow-up: Changed 11 years ago by jch

And you only want to do one hash check at a time, so only one worker thread is needed for that.

A worker thread is just a degenerate case of a thread pool -- it's just a thread pool with min=max=1. There's no reason why you couldn't use the thread pool interfaces to implement a worker thread cleanly.

If you have a worker thread implementation that you think might be better than a degenerate thread pool -- I'm interested.

The DNS lookups are done in a nonblocking manner

Charles, there's just no way around getaddrinfo if you want to do proper address selection. I'd much prefer we switched to using getaddrinfo in a dedicated thread (as I do in tr_dht) rather than reimplementing asynch DNS from scratch.

--Juliusz

comment:3 in reply to: ↑ 2 ; follow-up: Changed 11 years ago by charles

Replying to jch:

A worker thread is just a degenerate case of a thread pool -- it's just a thread pool with min=max=1. There's no reason why you couldn't use the thread pool interfaces to implement a worker thread cleanly.

My rationale is that it's overkill to add an entire framework for something that can be done in a few lines. When we need more concurrency then it may be worthwhile.

If you have a worker thread implementation that you think might be better than a degenerate thread pool -- I'm interested.

Give the IO queue a dedicated thread, create it once at startup. three or four lines of code.

Charles, there's just no way around getaddrinfo if you want to do proper address selection. I'd much prefer we switched to using getaddrinfo in a dedicated thread (as I do in tr_dht) rather than reimplementing asynch DNS from scratch.

We're using libevent's prewritten DNS lookup, not reimplementing it... but that's a minor point. The key issue is that if we let libcurl do its own getaddrinfo() calls, that lookup is going to block every other tracker announce being handled by libcurl. Running libcurl in a separate thread does not the problem.

comment:4 in reply to: ↑ 3 Changed 11 years ago by charles

Replying to charles:

If you have a worker thread implementation that you think might be better than a degenerate thread pool -- I'm interested.

Give the IO queue a dedicated thread, create it once at startup. three or four lines of code.

Rereading this, that answer answer was too glib -- the difference is in how one defines "better". I'm not arguing that a worker thread has better performance of functionality. They'd be about the same on those terms. However Transmission doesn't need more than that right now, so that's 500 fewer LOC that it has to rely on, that I don't have to dig through if there's a bug a year from now.

If Transmission adds more threads, then the threadpool code would be a better solution than multiple ad hoc worker threads.

comment:5 Changed 11 years ago by charles

  • Resolution set to invalid
  • Status changed from new to closed
  • Version changed from 1.82+ to 1.82

I've left this ticket open mostly to show deference to jch since I want him to keep contributing to DHT and Transmission ;) but I honestly don't think this is a good fit for libtransmission right now. libtransmission's current design, and foreseen future design, simply does not have that many use cases for threads.

The proposed threadpool's code looks small and good, and its license is good enough, so later on if libtransmission becomes more threaded then libtransmission should use this code.

comment:6 Changed 11 years ago by jch

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Summary changed from Threadpool infrastructure to Threading infrastructure

I'm retitling this bug and reopening it, since I believe we need to discuss it some more.

I think the main point that I'm making is that we need some sort of infrastructure to schedule work items in a worker thread, and to schedule work items' continuations in the libevent thread. Right now, the latter point is implemented, albeit in a somewhat hackish manner (runInEventThread), but there's no good infrastructure to do the former.

Charles, what interface do you suggest for doing that?

--Juliusz

comment:7 Changed 11 years ago by charles

How about mutexes, as is done with the task queueing mechanism in web.c?

comment:8 Changed 11 years ago by charles

  • Resolution set to invalid
  • Status changed from reopened to closed

Well since there wasn't any response, I'm not sure what there is to discuss. ;)

comment:9 follow-up: Changed 11 years ago by charles

  • Resolution invalid deleted
  • Status changed from closed to reopened

jch, a few suggestions:

  • threadpool_queue / threadpool_queue_t shouldn't be exposed in the header.
  • threadpool_item / threadpool_item_t shouldn't be defined in the header; it can be opaque.
  • trivial typo in threadpool.c: "every lock will ack" --> "every lock will act"

comment:10 in reply to: ↑ 9 Changed 10 years ago by jch

  • threadpool_queue / threadpool_queue_t shouldn't be exposed in the header.

Done.

  • threadpool_item / threadpool_item_t shouldn't be defined in the header; it can be opaque.

No. threadpool_items_run is just a utility function, you're allowed to rewrite it yourself (for example interleave execution with checking for timeouts). The alternative would be to provide a threadpool_item_run for a single item, but I'm lazy.

  • trivial typo in threadpool.c: "every lock will ack" --> "every lock will act"

Fixed.

Thanks,

--jch

comment:11 Changed 9 years ago by jordan

  • Resolution set to invalid
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.