BIND 10 #2873: Test the landlord/RCU approach for resolver multi-threading

[BIND 10 Development]

#2873: Test the landlord/RCU approach for resolver multi-threading
-------------------------------------+-------------------------------------
            Reporter:  vorner        |                        Owner:
                Type:  task          |  vorner
            Priority:  medium        |                       Status:
           Component:  resolver      |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-20130820
         Sub-Project:  DNS           |                   Resolution:
Estimated Difficulty:  6             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi Michal

 Here are my comments:

 * This seems to be an unedited copy in `landlord.h`:
 {{{
 /// \brief Naive implementation of resolver for the benchmark
 ///
 /// This is here mostly to show how to implement the other benchmark
 /// implementations. Look at the code inside how to use the fake
 /// resolution.
 }}}

 * In `NaiveResolver::run()`, use prefix `++` operator according to
   coding guidelines (though it doesn't make a practical difference
   here) to be consistent. Same in `main()` too in `for` loops.

 * It seems that this is not written to be run during check as it is a
   benchmark. When running `resolver-bench` manually, I was confused by
   the output when the last iterations took many seconds and I wondered
   what it was up to. If we want these experiments to live on in the
   tree, please section the output with headings (Landlord, Naive, etc.)
   so we know what implementation is being run as output is printed.

 * While it uses a landlord, this does not use RCU but uses a structure
   lock for interlocking which runs everything in lockstep. In an RCU
   implementation, `pop()` will not wait for the body of `push()` to
   complete and vice versa. RCU approach's benchmark results will be
   different if there's a benefit to using it.

 * This is benchmark code, so I don't know how much nitpicking is
   valid. I don't like how the return value of `pop()` is used to
   indicate shutdown. It's better to use a `isRunning()` method in the
   `while` loops, and in other places.

 * What happens in `read_iface()` if less than `read_batch_size_` queries
   are received? Is this written so because it is a benchmark, and a real
   implementation will timeout and `push()`/flush?

 * Is a second template parameter necessary in `LandlordResolver::Queue`?
   Why not simply have the following `typedef` inside `class
 LandlordResolver::Queue`:
 {{{
 typedef std::vector<T> Container;
 }}}

 * Can't `downstream_queue_` be made a stack variable between
   `checkUpstream()` and `completedUpstream()`? Its use and life seems to
   be within `checkUpstream()` codepath only.

 * Especially with how the condition variables are used in this branch, I
   propose creating another ticket that does the following. This will
   make such code cleaner without having to worry about the relationship
   between the mutex and the condition variable. The downside is we don't
   support `wait()` on unrelated mutexes, but I don't think we have such
   a usecase.
  * Make `Locker` constructor take a `Lockable` (interface) object that
    implements `lock()`, `tryLock()` and `unlock()` methods. Move
    `Locker` out of the `Mutex::` class.
  * Make `Mutex` and `CondVar` implement the `Lockable` interface.
  * Like the Python API, Make `CondVar` constructor take a `Mutex`
    argument and remove it from the `wait()` method arguments. If no
    mutex is passed, `CondVar` creates an internal `Mutex`. The
    `Lockable` implementation interface in `CondVar` wraps around the
    `Mutex` methods.
  * Remove `mutex_` from `LandlordResolver::Queue`. The code in `Queue`
    methods create `Locker` around the `CondVar`.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2873#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development