Merge lp:~therve/storm/cache-compile into lp:storm

[Thomas Herve]

Here it is. The branch adds a new attribute on Table and Column to cache the compilation. AFAIU, those compilations don't depend on the state, so we can store directly the value. It also changes the ClassInfo to store the table as a Table object instead of a SQLToken, to go through the compile_table logic.

I made a quick benchmark to show the gains:

                        | Trunk | Branch | Gain
10 inserts, 3 colums | 1.68s | 1.55s | 7.8%
100 insersts, 3 columns | 14.15s | 12.79s | 9.6%
20 insersts, 10 columns | 5.38s | 4.71s | 12.4%

I think it's worth it considering the low impact of the changes.

[Gustavo Niemeyer (niemeyer)]

Nice!

Would you mind to repeat the benchmark with 1000 inserts and 10 columns each, to see a bit better the effects? The first number feels a bit suspect (hard to believe 10 inserts would take 1.6 seconds).

review: Needs Information

[Thomas Herve (therve)]

Gustavo: sorry this was explicit, but it's run 1000 times over.

[Thomas Herve (therve)]

Here's the script I used, with the last run: http://pastebin.ubuntu.com/666307/

[Free Ekanayaka (free.ekanayaka)]

Hi Thomas,

that's indeed significantly faster for use cases similar to the one in the attached script.

+1

[1]

- self.table = SQLToken(self.table)
+ self.table = Table(self.table)

I guess this is to make use of the cache? Are SQLToken and Table fully interchangeable in this case?

[2]

It'd be good to add a couple of unit tests for the new behavior.

review: Approve

[lp:~therve/storm/cache-compile]

399. By Thomas Herve on 2011-08-24

Add tests for compile_cache value

[Thomas Herve (therve)]

[1] Yes, it's for using the cache. table has been documented as an expression, so I don't see any problem changing it. The compilation passes token=True which results to the same result.

[2] I added some tests.

Thanks!

[Jamu Kakar (jkakar)]

Looks good to me, +1!

review: Approve

[Gustavo Niemeyer (niemeyer)]

Thomas, I mentioned 1000 inserts, not running it 1000 times.

Either way, sounds like it's improved things in practice, so +1.

review: Approve

[Thomas Herve (therve)]

Right, with 1000 inserts

Branch: 0.227118968964
Trunk: 0.256669044495

So around 11% faster.

=== modified file 'storm/expr.py'
--- storm/expr.py	2011-05-16 10:39:36 +0000
+++ storm/expr.py	2011-08-24 11:55:39 +0000
@@ -800,7 +800,8 @@
     @ivar variable_factory: Factory producing C{Variable} instances typed
         according to this column.
     """
-    __slots__ = ("name", "table", "primary", "variable_factory")
+    __slots__ = ("name", "table", "primary", "variable_factory",
+                 "compile_cache")
 
     def __init__(self, name=Undef, table=Undef, primary=False,
                  variable_factory=None):
@@ -808,6 +809,7 @@
         self.table = table
         self.primary = int(primary)
         self.variable_factory = variable_factory or Variable
+        self.compile_cache = None
 
 @compile.when(Column)
 def compile_column(compile, column, state):
@@ -819,11 +821,15 @@
             alias = state.aliases.get(column)
             if alias is not None:
                 return compile(alias.name, state, token=True)
-        return compile(column.name, state, token=True)
+        if column.compile_cache is None:
+            column.compile_cache = compile(column.name, state, token=True)
+        return column.compile_cache
     state.push("context", COLUMN_PREFIX)
     table = compile(column.table, state, token=True)
     state.pop()
-    return "%s.%s" % (table, compile(column.name, state, token=True))
+    if column.compile_cache is None:
+        column.compile_cache = compile(column.name, state, token=True)
+    return "%s.%s" % (table, column.compile_cache)
 
 @compile_python.when(Column)
 def compile_python_column(compile, column, state):
@@ -870,14 +876,17 @@
 
 
 class Table(FromExpr):
-    __slots__ = ("name",)
+    __slots__ = ("name", "compile_cache")
 
     def __init__(self, name):
         self.name = name
+        self.compile_cache = None
 
 @compile.when(Table)
 def compile_table(compile, table, state):
-    return compile(table.name, state, token=True)
+    if table.compile_cache is None:
+        table.compile_cache = compile(table.name, state, token=True)
+    return table.compile_cache
 
 
 class JoinExpr(FromExpr):

=== modified file 'storm/info.py'
--- storm/info.py	2008-06-20 06:50:47 +0000
+++ storm/info.py	2011-08-24 11:55:39 +0000
@@ -18,11 +18,11 @@
 # You should have received a copy of the GNU Lesser General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #
-from weakref import ref, WeakKeyDictionary
+from weakref import ref
 
 from storm.exceptions import ClassInfoError
-from storm.expr import Expr, FromExpr, Column, Desc, TABLE
-from storm.expr import SQLToken, CompileError, compile
+from storm.expr import Column, Desc, TABLE
+from storm.expr import compile, Table
 from storm.event import EventSystem
 from storm import Undef, has_cextensions
 
@@ -40,9 +40,11 @@
         obj_info = ObjectInfo(obj)
         return obj.__dict__.setdefault("__storm_object_info__", obj_info)
 
+
 def set_obj_info(obj, obj_info):
     obj.__dict__["__storm_object_info__"] = obj_info
 
+
 def get_cls_info(cls):
     if "__storm_class_info__" in cls.__dict__:
         # Can't use attribute access here, otherwise subclassing won't work.
@@ -51,6 +53,7 @@
         cls.__storm_class_info__ = ClassInfo(cls)
         return cls.__storm_class_info__
 
+
 class ClassInfo(dict):
     """Persistent storm-related information of a class.
 
@@ -71,7 +74,7 @@
         self.cls = cls
 
         if isinstance(self.table, basestring):
-            self.table = SQLToken(self.table)
+            self.table = Table(self.table)
 
         pairs = []
         for attr in dir(cls):
@@ -79,7 +82,6 @@
             if isinstance(column, Column):
                 pairs.append((attr, column))
 
-
         pairs.sort()
 
         self.columns = tuple(pair[1] for pair in pairs)
@@ -121,7 +123,6 @@
         self.primary_key_pos = tuple(id_positions[id(column)]
                                      for column in self.primary_key)
 
-
         __order__ = getattr(cls, "__storm_order__", None)
         if __order__ is None:
             self.default_order = Undef
@@ -151,7 +152,7 @@
     __hash__ = object.__hash__
 
     # For get_obj_info(), an ObjectInfo is its own obj_info.
-    __storm_object_info__ = property(lambda self:self)
+    __storm_object_info__ = property(lambda self: self)
 
     def __init__(self, obj):
         # FASTPATH This method is part of the fast path.  Be careful when
@@ -171,7 +172,7 @@
                 column.variable_factory(column=column,
                                         event=event,
                                         validator_object_factory=self.get_obj)
- 
+
         self.primary_vars = tuple(variables[column]
                                   for column in self.cls_info.primary_key)
 
@@ -199,7 +200,6 @@
     from storm.cextensions import ObjectInfo, get_obj_info
 
 
-
 class ClassAlias(object):
     """Create a named alias for a Storm class for use in queries.
 
@@ -229,8 +229,7 @@
                 cls._storm_alias_cache = {}
             elif name in cache:
                 return cache[name]
-        cls_info = get_cls_info(cls)
-        alias_cls = type(cls.__name__+"Alias", (self_cls,),
+        alias_cls = type(cls.__name__ + "Alias", (self_cls,),
                          {"__storm_table__": name})
         alias_cls.__bases__ = (cls, self_cls)
         alias_cls_info = get_cls_info(alias_cls)

=== modified file 'tests/expr.py'
--- tests/expr.py	2011-05-11 05:38:41 +0000
+++ tests/expr.py	2011-08-24 11:55:39 +0000
@@ -135,6 +135,7 @@
         expr = Column()
         self.assertEquals(expr.name, Undef)
         self.assertEquals(expr.table, Undef)
+        self.assertIdentical(expr.compile_cache, None)
 
         # Test for identity. We don't want False there.
         self.assertTrue(expr.primary is 0)
@@ -1007,13 +1008,16 @@
         statement = compile(expr, state)
         self.assertEquals(statement, "column1")
         self.assertEquals(state.parameters, [])
+        self.assertEquals(expr.compile_cache, "column1")
 
     def test_column_table(self):
-        expr = Select(Column(column1, Func1()))
+        column = Column(column1, Func1())
+        expr = Select(column)
         state = State()
         statement = compile(expr, state)
         self.assertEquals(statement, "SELECT func1().column1 FROM func1()")
         self.assertEquals(state.parameters, [])
+        self.assertEquals(column.compile_cache, "column1")
 
     def test_column_contexts(self):
         table, = track_contexts(1)
@@ -1579,10 +1583,12 @@
 
     def test_table(self):
         expr = Table(table1)
+        self.assertIdentical(expr.compile_cache, None)
         state = State()
         statement = compile(expr, state)
         self.assertEquals(statement, '"table 1"')
         self.assertEquals(state.parameters, [])
+        self.assertEquals(expr.compile_cache, '"table 1"')
 
     def test_alias(self):
         expr = Alias(Table(table1), "name")

=== modified file 'tests/info.py'
--- tests/info.py	2008-06-18 17:28:37 +0000
+++ tests/info.py	2011-08-24 11:55:39 +0000
@@ -94,7 +94,7 @@
                           (self.Class.prop1, self.Class.prop2))
 
     def test_table(self):
-        self.assertEquals(self.cls_info.table, "table")
+        self.assertEquals(self.cls_info.table.name, "table")
 
     def test_primary_key(self):
         # Can't use == for props.
@@ -202,7 +202,7 @@
 
     def test_variables(self):
         self.assertTrue(isinstance(self.obj_info.variables, dict))
-        
+
         for column in self.cls_info.columns:
             variable = self.obj_info.variables.get(column)
             self.assertTrue(isinstance(variable, Variable))
@@ -227,7 +227,7 @@
 
     def test_primary_vars(self):
         self.assertTrue(isinstance(self.obj_info.primary_vars, tuple))
-        
+
         for column, variable in zip(self.cls_info.primary_key,
                                     self.obj_info.primary_vars):
             self.assertEquals(self.obj_info.variables.get(column),
@@ -316,7 +316,7 @@
         self.obj_info.checkpoint()
 
         obj = object()
-        
+
         self.obj_info.event.hook("changed", object_changed1, obj)
         self.obj_info.event.hook("changed", object_changed2, obj)
 
@@ -326,7 +326,7 @@
         self.assertEquals(changes1,
                   [(1, self.obj_info, self.variable2, Undef, 10, False, obj),
                    (1, self.obj_info, self.variable1, Undef, 20, False, obj)])
-        self.assertEquals(changes2,                                        
+        self.assertEquals(changes2,
                   [(2, self.obj_info, self.variable2, Undef, 10, False, obj),
                    (2, self.obj_info, self.variable1, Undef, 20, False, obj)])
 
@@ -339,7 +339,7 @@
         self.assertEquals(changes1,
                   [(1, self.obj_info, self.variable1, 20, None, False, obj),
                    (1, self.obj_info, self.variable2, 10, None, False, obj)])
-        self.assertEquals(changes2,                                       
+        self.assertEquals(changes2,
                   [(2, self.obj_info, self.variable1, 20, None, False, obj),
                    (2, self.obj_info, self.variable2, 10, None, False, obj)])
 
@@ -352,7 +352,7 @@
         self.assertEquals(changes1,
               [(1, self.obj_info, self.variable1, None, Undef, False, obj),
                (1, self.obj_info, self.variable2, None, Undef, False, obj)])
-        self.assertEquals(changes2,                                      
+        self.assertEquals(changes2,
               [(2, self.obj_info, self.variable1, None, Undef, False, obj),
                (2, self.obj_info, self.variable2, None, Undef, False, obj)])
 
@@ -528,11 +528,11 @@
             prop1 = Property("column1", primary=True)
         self.Class = Class
         self.ClassAlias = ClassAlias(self.Class, "alias")
-        
+
     def test_cls_info_cls(self):
         cls_info = get_cls_info(self.ClassAlias)
         self.assertEquals(cls_info.cls, self.Class)
-        self.assertEquals(cls_info.table, "alias")
+        self.assertEquals(cls_info.table.name, "alias")
         self.assertEquals(self.ClassAlias.prop1.name, "column1")
         self.assertEquals(self.ClassAlias.prop1.table, self.ClassAlias)