Merge lp:~roadmr/checkbox/lightning into lp:checkbox

[Daniel Manrique]

This branch adds a way for the two main objects dealing with persisting stuff to disk (MessageStore and Persist) to decide whether or not they should use "safe" file closing (which flushes the file buffer and syncs to disk to ensure data integrity).

The default (and very robust) behavior is to use safe file closing, just as it has been since this feature was introduced in rev 1090.

It then adds a few methods in the persist_info and jobs_prompt plugins to *disable* safeness prior to starting the gathering phase, and reenable it right after finishing gathering.

In principle this should cause no data loss, since the gathered data is obtained automatically and thus the process can be very easily repeated if needed.

The benefit of doing all this juggling is a significant speed increase in gathering. My system goes from 37-40 seconds on the gathering phase to just under 7 seconds with these changes.

I'm not entirely happy by the handling of Persist's _backend directly (in persist_info). Maybe adding a method to Persist proper to then set the backend's safeness would be more kosher. But this seemed so much easier!

[Marc Tardif (cr3)]

Very useful, thanks man! Just a couple comments about your code:

1. It is more Pythonish (or Pythonic?) to pass keyword arguments to function without spaces:

  foo(a=1, b=2).

2. In plugins/persist_info.py, you access private variables from the persist object:

  self.persist._backend.safe_file_closing = True

Instead, you might have liked to consider adding a public getter and setter on the Persist class:

  self.persist.setSafeFileClosing(True)

However, this doesn't prevent the code from working so I'm merging. All good!

review: Approve

=== modified file 'checkbox/contrib/persist.py'
--- checkbox/contrib/persist.py	2011-10-07 07:16:47 +0000
+++ checkbox/contrib/persist.py	2012-02-22 22:10:23 +0000
@@ -506,8 +506,11 @@
     def save(self, filepath, map):
         self._store[filepath] = map
 
-
-class PickleBackend(Backend):
+class DiskBackend(Backend):
+
+    safe_file_closing = True
+
+class PickleBackend(DiskBackend):
 
     def __init__(self):
         import cPickle
@@ -518,17 +521,17 @@
         try:
             return self._pickle.load(file)
         finally:
-            safe_close(file)
+            safe_close(file, self.safe_file_closing)
 
     def save(self, filepath, map):
         file = open(filepath, "w")
         try:
             self._pickle.dump(map, file, 2)
         finally:
-            safe_close(file)
-
-
-class BPickleBackend(Backend):
+            safe_close(file, self.safe_file_closing)
+
+
+class BPickleBackend(DiskBackend):
 
     def __init__(self):
         from checkbox.contrib import bpickle
@@ -539,11 +542,11 @@
         try:
             return self._bpickle.loads(file.read())
         finally:
-            safe_close(file)
+            safe_close(file, self.safe_file_closing)
 
     def save(self, filepath, map):
         file = open(filepath, "w")
         try:
             file.write(self._bpickle.dumps(map))
         finally:
-            safe_close(file)
+            safe_close(file, self.safe_file_closing)

=== modified file 'checkbox/lib/safe.py'
--- checkbox/lib/safe.py	2011-10-07 07:16:47 +0000
+++ checkbox/lib/safe.py	2012-02-22 22:10:23 +0000
@@ -95,7 +95,8 @@
 
     return md5sum
 
-def safe_close(file):
-    file.flush()
-    os.fsync(file.fileno())
+def safe_close(file, safe = True):
+    if safe:
+        file.flush()
+        os.fsync(file.fileno())
     file.close()

=== modified file 'checkbox/message.py'
--- checkbox/message.py	2012-02-14 20:32:39 +0000
+++ checkbox/message.py	2012-02-22 22:10:23 +0000
@@ -44,6 +44,10 @@
     #This caches everything but a message's data, making it manageable to keep in memory.
     _message_cache = {}
 
+    #Setting this to False speeds things up considerably, at the expense 
+    #of a higher risk of data loss during a crash
+    safe_file_closing = True
+
     def __init__(self, persist, directory, directory_size=1000):
         self._directory = directory
         self._directory_size = directory_size
@@ -219,7 +223,7 @@
         try:
             return file.read()
         finally:
-            safe_close(file)
+            safe_close(file, safe = self.safe_file_closing)
 
     def _get_flags(self, path):
         basename = posixpath.basename(path)
@@ -261,7 +265,7 @@
 
         file = open(filename + ".tmp", "w")
         file.write(message_data)
-        safe_close(file)
+        safe_close(file, safe = self.safe_file_closing)
 
         os.rename(filename + ".tmp", filename)
 

=== modified file 'debian/changelog'
--- debian/changelog	2012-02-22 21:05:54 +0000
+++ debian/changelog	2012-02-22 22:10:23 +0000
@@ -5,6 +5,8 @@
 
   [Daniel Manrique]
   * Use GObject from gi.repository instead of gobject (LP: #937099)
+  * Disable flushing to disk after every file access during gathering phase for
+    a significant speed boost. (LP: #939019)
 
   [Javier Collado]
   * Fixed running of disk/read_performance tests (LP: #933528)

=== modified file 'plugins/jobs_prompt.py'
--- plugins/jobs_prompt.py	2012-02-15 16:31:04 +0000
+++ plugins/jobs_prompt.py	2012-02-22 22:10:23 +0000
@@ -65,6 +65,13 @@
              ("report-job", self.report_job)]:
             self._manager.reactor.call_on(rt, rh)
 
+        #This should fire first thing during the gathering phase.
+        self._manager.reactor.call_on("gather", self.begin_gather, -900)
+
+        #This should fire last during gathering (i.e. after
+        #all other gathering callbacks are finished)
+        self._manager.reactor.call_on("gather", self.end_gather, 900)
+
     def begin_persist(self, persist):
         self._persist = persist
 
@@ -72,6 +79,14 @@
         if not recover:
             self.store.delete_all_messages()
 
+    def begin_gather(self):
+        #Speed boost during the gathering phase. Not critical data anyway.
+        self.store.safe_file_closing = False
+
+    def end_gather(self):
+        #Back to saving data very carefully once gathering is done.
+        self.store.safe_file_closing = True 
+
     def ignore_jobs(self, jobs):
         self._ignore = jobs
 

=== modified file 'plugins/persist_info.py'
--- plugins/persist_info.py	2011-10-03 10:50:20 +0000
+++ plugins/persist_info.py	2012-02-22 22:10:23 +0000
@@ -42,6 +42,13 @@
         # Save persist data last
         self._manager.reactor.call_on("stop", self.save, 1000)
 
+        #This should fire first thing during the gathering phase.
+        self._manager.reactor.call_on("gather", self.begin_gather, -900)
+
+        #This should fire last during gathering (i.e. after
+        #all other gathering callbacks are finished)
+        self._manager.reactor.call_on("gather", self.end_gather, 900)
+
     def begin(self, interface=None):
         if self.persist is None:
             self.persist = Persist(self.filename)
@@ -52,5 +59,12 @@
         if self.persist:
             self.persist.save()
 
+    def begin_gather(self):
+        #Speed boost during the gathering phase. Not critical data anyway.
+        self.persist._backend.safe_file_closing = False
+
+    def end_gather(self):
+        #Back to saving data very carefully once gathering is done.
+        self.persist._backend.safe_file_closing = True 
 
 factory = PersistInfo