From bf944144b1283d93a5138145cbdb576cea7a9fa0 Mon Sep 17 00:00:00 2001
From: Michal 'vorner' Vaner <michal.vaner@nic.cz>
Date: Tue, 1 Feb 2011 12:39:23 +0100
Subject: [PATCH] [trac537] Some explaining comments

---
 src/lib/asiolink/internal/udpdns.h |  9 ++++++
 src/lib/asiolink/udpdns.cc         | 44 ++++++++++++++++++++++++++----
 2 files changed, 47 insertions(+), 6 deletions(-)

diff --git a/src/lib/asiolink/internal/udpdns.h b/src/lib/asiolink/internal/udpdns.h
index d8bffa9b3..5b67ca95b 100644
--- a/src/lib/asiolink/internal/udpdns.h
+++ b/src/lib/asiolink/internal/udpdns.h
@@ -173,6 +173,15 @@ public:
 private:
     enum { MAX_LENGTH = 4096 };
 
+    /**
+     * \brief Internal state and data.
+     *
+     * We use the pimple design pattern, but not because we need to hide
+     * internal data. This class and whole header is for private use anyway.
+     * It turned out that UDPServer is copied a lot, because it is a coroutine.
+     * This way the overhead of copying is lower, we copy only one shared
+     * pointer instead of about 10 of them.
+     */
     class Data;
     boost::shared_ptr<Data> data_;
 };
diff --git a/src/lib/asiolink/udpdns.cc b/src/lib/asiolink/udpdns.cc
index 63d1fa9b7..56bcee24b 100644
--- a/src/lib/asiolink/udpdns.cc
+++ b/src/lib/asiolink/udpdns.cc
@@ -48,7 +48,20 @@ using namespace isc::dns;
 
 namespace asiolink {
 
+/*
+ * Some of the member variables here are shared_ptrs and some are
+ * auto_ptrs. There will be one instance of Data for the lifetime
+ * of packet. The variables that are state only for a single packet
+ * use auto_ptr, as it is more lightweight. In the case of shared
+ * configuration (eg. the callbacks, socket), we use shared_ptrs.
+ */
 struct UDPServer::Data {
+    /*
+     * Constructor from parameters passed to UDPServer constructor.
+     * This instance will not be used to retrieve and answer the actual
+     * query, it will only hold parameters until we wait for the
+     * first packet. But we do initialize the socket in here.
+     */
     Data(io_service& io_service, const ip::address& addr, const uint16_t port,
         SimpleCallback* checkin, DNSLookup* lookup, DNSAnswer* answer) :
         io_(io_service), done_(false), checkin_callback_(checkin),
@@ -65,6 +78,13 @@ struct UDPServer::Data {
         socket_->bind(udp::endpoint(addr, port));
     }
 
+    /*
+     * Copy constructor. Default one would probably do, but it is unnecessary
+     * to copy many of the member variables every time we fork to handle
+     * another packet.
+     *
+     * We also allocate data for receiving the packet here.
+     */
     Data(const Data& other) :
         io_(other.io_), socket_(other.socket_), done_(false),
         checkin_callback_(other.checkin_callback_),
@@ -94,7 +114,7 @@ struct UDPServer::Data {
     // constructor, or in the function operator while processing a query.
     // Repeated allocations from the heap for every incoming query is
     // clearly a performance issue; this must be optimized in the future.
-    // The plan is to have a structure pre-allocate several "server state"
+    // The plan is to have a structure pre-allocate several "Data"
     // objects which can be pulled off a free list and placed on an in-use
     // list whenever a query comes in.  This will serve the dual purpose
     // of improving performance and guaranteeing that state information
@@ -141,7 +161,8 @@ struct UDPServer::Data {
 
 /// The following functions implement the \c UDPServer class.
 ///
-/// The constructor
+/// The constructor. It just creates new internal state object
+/// and lets it handle the initialization.
 UDPServer::UDPServer(io_service& io_service, const ip::address& addr,
     const uint16_t port, SimpleCallback* checkin, DNSLookup* lookup,
     DNSAnswer* answer) :
@@ -158,6 +179,11 @@ UDPServer::operator()(error_code ec, size_t length) {
 
     CORO_REENTER (this) {
         do {
+            /*
+             * This is preparation for receiving a packet. We get a new
+             * state object for the lifetime of the next packet to come.
+             * It allocates the buffers to receive data into.
+             */
             data_.reset(new Data(*data_));
 
             do {
@@ -171,10 +197,16 @@ UDPServer::operator()(error_code ec, size_t length) {
 
             data_->bytes_ = length;
 
-            /// Fork the coroutine by creating a copy of this one and
-            /// scheduling it on the ASIO service queue.  The parent
-            /// will continue listening for DNS packets while the child
-            /// processes the one that has just arrived.
+            /*
+             * We fork the coroutine now. One (the child) will keep
+             * the current state and handle the packet, then die and
+             * drop ownership of the state. The other (parent) will just
+             * go into the loop again and replace the current state with
+             * a new one for a new object.
+             *
+             * Actually, both of the coroutines will be a copy of this
+             * one, but that's just internal implementation detail.
+             */
             CORO_FORK data_->io_.post(UDPServer(*this));
         } while (is_parent());
 
-- 
GitLab