Performance Enhancements

- Nibbler: Now locally stores input length rather than repeatedly calling
  std::string::length.
- Nibbler: Makes greater use of std::string::find_first_not_of, instead of
  looping.
- TDB: No longer applies empty filters to lists - just copies the lists.
- TDB: Now caches data when reading completed.data.
- TDB: During loadPending and loadCompleted, makes fewer copies of the data.
- TDB: In commit, breaks out of search loops after finding the right data.
- TDB: In gc, only writes out minimal pending or completed data, instead of
       all data, all the time.
- TDB: No longer reads completed.data in gc, and simply appends completed
       and deleted tasks to it.

NEWS

@@ -8,6 +8,7 @@ New Features in task 1.9
   - New 'show' command to display configuration settings.
   - New 'denotate' command to delete annotations.
   - New limit:page filter to show only one page of tasks.
+  - Performance enhancements.
 
   Please refer to the ChangeLog file for full details.  There are too many to
   list here.

src/Nibbler.cpp

@@ -26,12 +26,16 @@
 ////////////////////////////////////////////////////////////////////////////////
 
 #include <stdlib.h>
+#include <string.h>
 #include <ctype.h>
 #include "Nibbler.h"
 
+const char* c_digits = "0123456789";
+
 ////////////////////////////////////////////////////////////////////////////////
 Nibbler::Nibbler ()
 : mInput ("")
+, mLength (0)
 , mCursor (0)
 {
 }
@@ -39,6 +43,7 @@ Nibbler::Nibbler ()
 ////////////////////////////////////////////////////////////////////////////////
 Nibbler::Nibbler (const char* input)
 : mInput (input)
+, mLength (strlen (input))
 , mCursor (0)
 {
 }
@@ -46,6 +51,7 @@ Nibbler::Nibbler (const char* input)
 ////////////////////////////////////////////////////////////////////////////////
 Nibbler::Nibbler (const std::string& input)
 : mInput (input)
+, mLength (input.length ())
 , mCursor (0)
 {
 }
@@ -54,6 +60,7 @@ Nibbler::Nibbler (const std::string& input)
 Nibbler::Nibbler (const Nibbler& other)
 {
   mInput  = other.mInput;
+  mLength = other.mLength;
   mCursor = other.mCursor;
 }
 
@@ -63,6 +70,7 @@ Nibbler& Nibbler::operator= (const Nibbler& other)
   if (this != &other)
   {
     mInput  = other.mInput;
+    mLength = other.mLength;
     mCursor = other.mCursor;
   }
 
@@ -78,7 +86,7 @@ Nibbler::~Nibbler ()
 // Extract up until the next c, or EOS.
 bool Nibbler::getUntil (char c, std::string& result)
 {
-  if (mCursor < mInput.length ())
+  if (mCursor < mLength)
   {
     std::string::size_type i = mInput.find (c, mCursor);
     if (i != std::string::npos)
@@ -89,7 +97,7 @@ bool Nibbler::getUntil (char c, std::string& result)
     else
     {
       result = mInput.substr (mCursor);
-      mCursor = mInput.length ();
+      mCursor = mLength;
     }
 
     return true;
@@ -101,7 +109,7 @@ bool Nibbler::getUntil (char c, std::string& result)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::getUntil (const std::string& terminator, std::string& result)
 {
-  if (mCursor < mInput.length ())
+  if (mCursor < mLength)
   {
     std::string::size_type i = mInput.find (terminator, mCursor);
     if (i != std::string::npos)
@@ -112,7 +120,7 @@ bool Nibbler::getUntil (const std::string& terminator, std::string& result)
     else
     {
       result = mInput.substr (mCursor);
-      mCursor = mInput.length ();
+      mCursor = mLength;
     }
 
     return true;
@@ -124,7 +132,7 @@ bool Nibbler::getUntil (const std::string& terminator, std::string& result)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::getUntilOneOf (const std::string& chars, std::string& result)
 {
-  if (mCursor < mInput.length ())
+  if (mCursor < mLength)
   {
     std::string::size_type i = mInput.find_first_of (chars, mCursor);
     if (i != std::string::npos)
@@ -135,7 +143,7 @@ bool Nibbler::getUntilOneOf (const std::string& chars, std::string& result)
     else
     {
       result = mInput.substr (mCursor);
-      mCursor = mInput.length ();
+      mCursor = mLength;
     }
 
     return true;
@@ -147,10 +155,10 @@ bool Nibbler::getUntilOneOf (const std::string& chars, std::string& result)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::skipN (const int quantity /* = 1 */)
 {
-  if (mCursor >= mInput.length ())
+  if (mCursor >= mLength)
     return false;
 
-  if (mCursor <= mInput.length () - quantity)
+  if (mCursor <= mLength - quantity)
   {
     mCursor += quantity;
     return true;
@@ -162,7 +170,7 @@ bool Nibbler::skipN (const int quantity /* = 1 */)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::skip (char c)
 {
-  if (mCursor < mInput.length () &&
+  if (mCursor < mLength &&
       mInput[mCursor] == c)
   {
     ++mCursor;
@@ -175,13 +183,17 @@ bool Nibbler::skip (char c)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::skipAll (char c)
 {
-  std::string::size_type i = mCursor;
-  while (i < mInput.length () && mInput[i] == c)
-    ++i;
-
-  if (i != mCursor)
+  if (mCursor < mLength)
   {
+    std::string::size_type i = mInput.find_first_not_of (c, mCursor);
+    if (i == mCursor)
+      return false;
+
+    if (i == std::string::npos)
+      mCursor = mLength;  // Yes, off the end.
+    else
       mCursor = i;
+
     return true;
   }
 
@@ -191,14 +203,14 @@ bool Nibbler::skipAll (char c)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::skipAllOneOf (const std::string& chars)
 {
-  if (mCursor < mInput.length ())
+  if (mCursor < mLength)
   {
     std::string::size_type i = mInput.find_first_not_of (chars, mCursor);
     if (i == mCursor)
       return false;
 
     if (i == std::string::npos)
-      mCursor = mInput.length ();  // Yes, off the end.
+      mCursor = mLength;  // Yes, off the end.
     else
       mCursor = i;
 
@@ -212,10 +224,10 @@ bool Nibbler::skipAllOneOf (const std::string& chars)
 bool Nibbler::getQuoted (char c, std::string& result)
 {
   std::string::size_type start = mCursor;
-  if (start < mInput.length () && mInput[start] == c)
+  if (start < mLength && mInput[start] == c)
   {
     ++start;
-    if (start < mInput.length ())
+    if (start < mLength)
     {
       std::string::size_type end = mInput.find (c, start);
       if (end != std::string::npos)
@@ -235,7 +247,7 @@ bool Nibbler::getInt (int& result)
 {
   std::string::size_type i = mCursor;
 
-  if (i < mInput.length ())
+  if (i < mLength)
   {
     if (mInput[i] == '-')
       ++i;
@@ -243,7 +255,8 @@ bool Nibbler::getInt (int& result)
       ++i;
   }
 
-  while (i < mInput.length () && isdigit (mInput[i]))
+  // TODO Potential for use of find_first_not_of
+  while (i < mLength && isdigit (mInput[i]))
     ++i;
 
   if (i > mCursor)
@@ -260,7 +273,8 @@ bool Nibbler::getInt (int& result)
 bool Nibbler::getUnsignedInt (int& result)
 {
   std::string::size_type i = mCursor;
-  while (i < mInput.length () && isdigit (mInput[i]))
+  // TODO Potential for use of find_first_not_of
+  while (i < mLength && isdigit (mInput[i]))
     ++i;
 
   if (i > mCursor)
@@ -282,10 +296,10 @@ bool Nibbler::getUntilEOL (std::string& result)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::getUntilEOS (std::string& result)
 {
-  if (mCursor < mInput.length ())
+  if (mCursor < mLength)
   {
     result = mInput.substr (mCursor);
-    mCursor = mInput.length ();
+    mCursor = mLength;
     return true;
   }
 
@@ -295,7 +309,7 @@ bool Nibbler::getUntilEOS (std::string& result)
 ////////////////////////////////////////////////////////////////////////////////
 bool Nibbler::depleted ()
 {
-  if (mCursor >= mInput.length ())
+  if (mCursor >= mLength)
     return true;
 
   return false;

src/Nibbler.h

@@ -55,6 +55,7 @@ public:
 
 private:
   std::string mInput;
+  std::string::size_type mLength;
   std::string::size_type mCursor;
 };
 

src/TDB.cpp

@@ -219,6 +219,7 @@ int TDB::loadPending (std::vector <Task>& tasks, Filter& filter)
 
   try
   {
+    // Only load if not already loaded.
     if (mPending.size () == 0)
     {
       mId = 1;
@@ -247,13 +248,23 @@ int TDB::loadPending (std::vector <Task>& tasks, Filter& filter)
     }
 
     // Now filter and return.
+    if (filter.size ())
+    {
       foreach (task, mPending)
         if (filter.pass (*task))
           tasks.push_back (*task);
+    }
+    else
+    {
+      foreach (task, mPending)
+        tasks.push_back (*task);
+    }
 
     // Hand back any accumulated additions, if TDB::loadPending is being called
     // repeatedly.
     int fakeId = mId;
+    if (filter.size ())
+    {
       foreach (task, mNew)
       {
         task->id = fakeId++;
@@ -261,6 +272,15 @@ int TDB::loadPending (std::vector <Task>& tasks, Filter& filter)
           tasks.push_back (*task);
       }
     }
+    else
+    {
+      foreach (task, mNew)
+      {
+        task->id = fakeId++;
+        tasks.push_back (*task);
+      }
+    }
+  }
 
   catch (std::string& e)
   {
@@ -281,9 +301,11 @@ int TDB::loadCompleted (std::vector <Task>& tasks, Filter& filter)
   Timer t ("TDB::loadCompleted");
 
   std::string file;
-  int line_number;
+  int line_number = 1;
 
   try
+  {
+    if (mCompleted.size () == 0)
     {
       char line[T_LINE_MAX];
       foreach (location, mLocations)
@@ -299,14 +321,10 @@ int TDB::loadCompleted (std::vector <Task>& tasks, Filter& filter)
           {
             // TODO Add hidden attribute indicating source?
 
-          if (line[length - 1] == '\n')
-            line[length - 1] = '\0';
-
             Task task (line);
             task.id = 0;  // Need a value, just not a valid value.
 
-          if (filter.pass (task))
-            tasks.push_back (task);
+            mCompleted.push_back (task);
           }
 
           ++line_number;
@@ -314,6 +332,20 @@ int TDB::loadCompleted (std::vector <Task>& tasks, Filter& filter)
       }
     }
 
+    // Now filter and return.
+    if (filter.size ())
+    {
+      foreach (task, mCompleted)
+        if (filter.pass (*task))
+          tasks.push_back (*task);
+    }
+    else
+    {
+      foreach (task, mCompleted)
+        tasks.push_back (*task);
+    }
+  }
+
   catch (std::string& e)
   {
     std::stringstream s;
@@ -329,7 +361,6 @@ int TDB::loadCompleted (std::vector <Task>& tasks, Filter& filter)
 void TDB::add (const Task& task)
 {
   mNew.push_back (task);
-
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -344,7 +375,7 @@ void TDB::update (const Task& task)
 int TDB::commit ()
 {
   Timer t ("TDB::commit");
-  context.hooks.trigger ("pre-gc");
+  context.hooks.trigger ("pre-commit");
 
   int quantity = mNew.size () + mModified.size ();
 
@@ -364,7 +395,7 @@ int TDB::commit ()
       writeUndo (*task, mLocations[0].undo);
 
     mNew.clear ();
-    context.hooks.trigger ("post-gc");
+    context.hooks.trigger ("post-commit");
     return quantity;
   }
 
@@ -375,10 +406,20 @@ int TDB::commit ()
     // new tasks appended.
     std::vector <Task> allPending;
     allPending = mPending;
-    foreach (task, allPending)
     foreach (mtask, mModified)
+    {
+      foreach (task, allPending)
+      {
         if (task->id == mtask->id)
+        {
           *task = *mtask;
+          goto next_mod;
+        }
+      }
+
+    next_mod:
+      ;
+    }
 
     foreach (task, mNew)
       allPending.push_back (*task);
@@ -396,10 +437,20 @@ int TDB::commit ()
     // Update the undo log.
     if (fseek (mLocations[0].undo, 0, SEEK_END) == 0)
     {
-      foreach (task, mPending)
       foreach (mtask, mModified)
+      {
+        foreach (task, mPending)
+        {
           if (task->id == mtask->id)
+          {
             writeUndo (*task, *mtask, mLocations[0].undo);
+            goto next_mod2;
+          }
+        }
+
+      next_mod2:
+        ;
+      }
 
       foreach (task, mNew)
         writeUndo (*task, mLocations[0].undo);
@@ -411,7 +462,7 @@ int TDB::commit ()
     mNew.clear ();
   }
 
-  context.hooks.trigger ("post-gc");
+  context.hooks.trigger ("post-commit");
   return quantity;
 }
 
@@ -423,43 +474,45 @@ int TDB::gc ()
 {
   Timer t ("TDB::gc");
 
-  int count = 0;
+  int count_pending_changes = 0;
+  int count_completed_changes = 0;
   Date now;
 
-  // Set up a second TDB.
+  if (mNew.size ())
+    throw std::string ("Unexpected new tasks found during gc.");
+
+  if (mModified.size ())
+    throw std::string ("Unexpected modified tasks found during gc.");
+
+  lock ();
+
   Filter filter;
-  TDB tdb;
-  tdb.location (mLocations[0].path);
-  tdb.lock ();
-
-  std::vector <Task> pending;
-  tdb.loadPending (pending, filter);
-
-  std::vector <Task> completed;
-  tdb.loadCompleted (completed, filter);
+  std::vector <Task> ignore;
+  loadPending (ignore, filter);
 
   // Now move completed and deleted tasks from the pending list to the
   // completed list.  Isn't garbage collection easy?
   std::vector <Task> still_pending;
-  foreach (task, pending)
+  std::vector <Task> newly_completed;
+  foreach (task, mPending)
   {
-    std::string st = task->get ("status");
     Task::status s = task->getStatus ();
     if (s == Task::completed ||
         s == Task::deleted)
     {
-      completed.push_back (*task);
-      ++count;
+      newly_completed.push_back (*task);
+      ++count_pending_changes;    // removal
+      ++count_completed_changes;  // addition
     }
     else if (s == Task::waiting)
     {
-      // Wake up tasks that are waiting.
+      // Wake up tasks that need to be woken.
       Date wait_date (atoi (task->get ("wait").c_str ()));
       if (now > wait_date)
       {
         task->setStatus (Task::pending);
         task->remove ("wait");
-        ++count;
+        ++count_pending_changes;  // modification
       }
 
       still_pending.push_back (*task);
@@ -468,37 +521,38 @@ int TDB::gc ()
       still_pending.push_back (*task);
   }
 
-  pending = still_pending;
-
   // No commit - all updates performed manually.
-  if (count > 0)
+  if (count_pending_changes > 0)
   {
-    if (fseek (tdb.mLocations[0].pending, 0, SEEK_SET) == 0)
+    if (fseek (mLocations[0].pending, 0, SEEK_SET) == 0)
     {
-      if (ftruncate (fileno (tdb.mLocations[0].pending), 0))
+      if (ftruncate (fileno (mLocations[0].pending), 0))
         throw std::string ("Failed to truncate pending.data file ");
 
-      foreach (task, pending)
-        fputs (task->composeF4 ().c_str (), tdb.mLocations[0].pending);
+      foreach (task, still_pending)
+        fputs (task->composeF4 ().c_str (), mLocations[0].pending);
+
+      // Update cached copy.
+      mPending = still_pending;
+    }
   }
 
-    if (fseek (tdb.mLocations[0].completed, 0, SEEK_SET) == 0)
+  // Append the new_completed tasks to completed.data.  No need to write out the
+  // whole list.
+  if (count_completed_changes > 0)
   {
-      if (ftruncate (fileno (tdb.mLocations[0].completed), 0))
-        throw std::string ("Failed to truncate completed.data file ");
-
-      foreach (task, completed)
-        fputs (task->composeF4 ().c_str (), tdb.mLocations[0].completed);
-    }
+    fseek (mLocations[0].completed, 0, SEEK_END);
+    foreach (task, newly_completed)
+      fputs (task->composeF4 ().c_str (), mLocations[0].completed);
   }
 
   // Close files.
-  tdb.unlock ();
+  unlock ();
 
   std::stringstream s;
-  s << "gc " << count << " tasks";
+  s << "gc " << (count_pending_changes + count_completed_changes) << " tasks";
   context.debug (s.str ());
-  return count;
+  return count_pending_changes + count_completed_changes;
 }
 
 ////////////////////////////////////////////////////////////////////////////////

src/TDB.h

@@ -75,11 +75,9 @@ private:
   int mId;
 
   std::vector <Task> mPending;   // Contents of pending.data
-
+  std::vector <Task> mCompleted; // Contents of pending.data
   std::vector <Task> mNew;       // Uncommitted new tasks
   std::vector <Task> mModified;  // Uncommitted modified tasks
-
-  // TODO Need cache of raw file contents to preserve comments.
 };
 
 #endif

src/tests/tdb.t.cpp

@@ -71,17 +71,19 @@ int main (int argc, char** argv)
     Task task ("[name:\"value\"]");
     tdb.add (task);
     tdb.unlock ();
+// P0 C0 N1 M0
 
     pending.clear ();
     completed.clear ();
     get (pending, completed);
+
     t.ok (pending.size () == 0, "TDB add -> no commit -> empty");
     t.ok (completed.size () == 0, "TDB add -> no commit -> empty");
 
     // Add with commit.
     tdb.lock ();
-    tdb.add (task);
-    tdb.commit ();
+    tdb.add (task);                                                  // P0 C0 N1 M0
+    tdb.commit ();                                                   // P1 C0 N0 M0
     tdb.unlock ();
 
     get (pending, completed);
@@ -91,36 +93,40 @@ int main (int argc, char** argv)
     t.ok (completed.size () == 0, "TDB add -> commit -> saved");
 
     // Update with commit.
-    tdb.lock ();
     pending.clear ();
     completed.clear ();
+
+    tdb.lock ();
     tdb.load (all, context.filter);
     all[0].set ("name", "value2");
-    tdb.update (all[0]);
-    tdb.commit ();
+    tdb.update (all[0]);                                             // P1 C0 N0 M1
+    tdb.commit ();                                                   // P1 C0 N0 M0
     tdb.unlock ();
 
     pending.clear ();
     completed.clear ();
     get (pending, completed);
+
     t.ok (all.size () == 1, "TDB update -> commit -> saved");
     t.is (all[0].get ("name"), "value2", "TDB load name=value2");
     t.is (all[0].id, 1, "TDB load verification id=1");
 
     // GC.
-    tdb.lock ();
     all.clear ();
+
+    tdb.lock ();
     tdb.loadPending (all, context.filter);
     all[0].setStatus (Task::completed);
-    tdb.update (all[0]);
+    tdb.update (all[0]);                                             // P1 C0 N0 M1
     Task t2 ("[foo:\"bar\" status:\"pending\"]");
-    tdb.add (t2);
+    tdb.add (t2);                                                    // P1 C0 N1 M1
     tdb.commit ();
     tdb.unlock ();
 
     pending.clear ();
     completed.clear ();
     get (pending, completed);
+
     t.is (pending.size (), (size_t)2,               "TDB before gc pending #2");
     t.is (pending[0].id, 1,                         "TDB before gc pending id 1");
     t.is (pending[0].getStatus (), Task::completed, "TDB before gc pending status completed");
@@ -128,11 +134,12 @@ int main (int argc, char** argv)
     t.is (pending[1].getStatus (), Task::pending,   "TDB before gc pending status pending");
     t.is (completed.size (), (size_t)0,             "TDB before gc completed 0");
 
-    tdb.gc ();
+    tdb.gc ();                                                       // P1 C1 N0 M0
 
     pending.clear ();
     completed.clear ();
     get (pending, completed);
+
     t.is (pending.size (), (size_t)1,                 "TDB after gc pending #1");
     t.is (pending[0].id, 1,                           "TDB after gc pending id 2");
     t.is (pending[0].getStatus (), Task::pending,     "TDB after gc pending status pending");