Don't apply aliasing + adaption for simple relationship joins

Identified a performance issue in the system by which a join is constructed
based on a mapped relationship.   The clause adaption system would be used
for the majority of join expressions including in the common case where no
adaptation is needed.   The conditions under which this adaptation occur
have been refined so that average non-aliased joins along a simple
relationship without a "secondary" table use about 70% less function calls.

Change-Id: Ifbe04214576e5a9fac86ca80c1dc7145c27cd50a

diff --git a/doc/build/changelog/unreleased_13/rel_join.rst b/doc/build/changelog/unreleased_13/rel_join.rst
new file mode 100644 (file)
index 0000000..1f0e29e
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/rel_join.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: performance, orm
+
+    Identified a performance issue in the system by which a join is constructed
+    based on a mapped relationship.   The clause adaption system would be used
+    for the majority of join expressions including in the common case where no
+    adaptation is needed.   The conditions under which this adaptation occur
+    have been refined so that average non-aliased joins along a simple
+    relationship without a "secondary" table use about 70% less function calls.
+

diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index fc3e78ea1d1d3738afc9a47258564761c02bdb30..7ee12ca6c3d598acfe50d3a5c8b6366061f0d1f1 100644 (file)
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -1044,6 +1044,7 @@ class RelationshipProperty(StrategizedProperty):
                 source_selectable=adapt_from,
                 source_polymorphic=True,
                 of_type_mapper=of_type_mapper,
+                alias_secondary=True,
             )
             if sj is not None:
                 return pj & sj
@@ -2215,12 +2216,18 @@ class RelationshipProperty(StrategizedProperty):
         dest_polymorphic=False,
         dest_selectable=None,
         of_type_mapper=None,
+        alias_secondary=False,
     ):
+
+        aliased = False
+
+        if alias_secondary and self.secondary is not None:
+            aliased = True
+
         if source_selectable is None:
             if source_polymorphic and self.parent.with_polymorphic:
                 source_selectable = self.parent._with_polymorphic_selectable
 
-        aliased = False
         if dest_selectable is None:
             dest_selectable = self.entity.selectable
             if dest_polymorphic and self.mapper.with_polymorphic:
@@ -2229,13 +2236,20 @@ class RelationshipProperty(StrategizedProperty):
             if self._is_self_referential and source_selectable is None:
                 dest_selectable = dest_selectable._anonymous_fromclause()
                 aliased = True
-        else:
+        elif dest_selectable is not self.mapper._with_polymorphic_selectable:
             aliased = True
 
         dest_mapper = of_type_mapper or self.mapper
 
         single_crit = dest_mapper._single_table_criterion
-        aliased = aliased or (source_selectable is not None)
+        aliased = aliased or (
+            source_selectable is not None
+            and (
+                source_selectable
+                is not self.parent._with_polymorphic_selectable
+                or source_selectable._is_subquery
+            )
+        )
 
         (
             primaryjoin,

diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index b96387c08dfa454e06fe25ffc772149c82a4707d..2f78fa5351751595d1632fed7cba85fea7446fda 100644 (file)
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -1035,6 +1035,7 @@ class _ORMJoin(expression.Join):
                 source_polymorphic=True,
                 dest_polymorphic=True,
                 of_type_mapper=right_info.mapper,
+                alias_secondary=True,
             )
 
             if sj is not None:

diff --git a/lib/sqlalchemy/testing/profiling.py b/lib/sqlalchemy/testing/profiling.py
index bfc4997e132cc2e61235a82fb7ab49839dc82f7e..b837d9b5b579ae4bc9e6376209a4b05e5c596385 100644 (file)
--- a/lib/sqlalchemy/testing/profiling.py
+++ b/lib/sqlalchemy/testing/profiling.py
@@ -221,7 +221,7 @@ class ProfileStatsFile(object):
         profile_f.close()
 
 
-def function_call_count(variance=0.05):
+def function_call_count(variance=0.05, times=1):
     """Assert a target for a test case's function call count.
 
     The main purpose of this assertion is to detect changes in
@@ -234,8 +234,11 @@ def function_call_count(variance=0.05):
 
     def decorate(fn):
         def wrap(*args, **kw):
+            timerange = range(times)
             with count_functions(variance=variance):
-                return fn(*args, **kw)
+                for time in timerange:
+                    rv = fn(*args, **kw)
+                return rv
 
         return update_wrapper(wrap, fn)
 

diff --git a/test/aaa_profiling/test_orm.py b/test/aaa_profiling/test_orm.py
index 209bc02e3f37a4e7d0bade6fe760a839b91da2d5..e2df85a33997db185c4647c34cdb9a3ca624b581 100644 (file)
--- a/test/aaa_profiling/test_orm.py
+++ b/test/aaa_profiling/test_orm.py
@@ -1,12 +1,16 @@
+from sqlalchemy import and_
 from sqlalchemy import ForeignKey
 from sqlalchemy import inspect
 from sqlalchemy import Integer
+from sqlalchemy import join
 from sqlalchemy import String
 from sqlalchemy import testing
+from sqlalchemy.orm import aliased
 from sqlalchemy.orm import Bundle
 from sqlalchemy.orm import configure_mappers
 from sqlalchemy.orm import defaultload
 from sqlalchemy.orm import defer
+from sqlalchemy.orm import join as orm_join
 from sqlalchemy.orm import joinedload
 from sqlalchemy.orm import Load
 from sqlalchemy.orm import mapper
@@ -818,6 +822,93 @@ class JoinedEagerLoadTest(fixtures.MappedTest):
         go()
 
 
+class JoinConditionTest(fixtures.DeclarativeMappedTest):
+    @classmethod
+    def setup_classes(cls):
+        class A(cls.DeclarativeBasic):
+            __tablename__ = "a"
+
+            id = Column(Integer, primary_key=True)
+            b_id = Column(ForeignKey("b.id"))
+            b = relationship("B")
+
+        class B(cls.DeclarativeBasic):
+            __tablename__ = "b"
+
+            id = Column(Integer, primary_key=True)
+            d_id = Column(ForeignKey("d.id"))
+
+        class C(cls.DeclarativeBasic):
+            __tablename__ = "c"
+
+            id = Column(Integer, primary_key=True)
+            a_id = Column(ForeignKey("a.id"))
+            d_id = Column(ForeignKey("d.id"))
+
+        class D(cls.DeclarativeBasic):
+            __tablename__ = "d"
+
+            id = Column(Integer, primary_key=True)
+
+        j = join(B, D, B.d_id == D.id).join(C, C.d_id == D.id)
+
+        A.d = relationship(
+            "D",
+            secondary=j,
+            primaryjoin=and_(A.b_id == B.id, A.id == C.a_id),
+            secondaryjoin=D.id == B.d_id,
+            uselist=False,
+            viewonly=True,
+        )
+
+    def test_a_to_b_plain(self):
+        A, B = self.classes("A", "B")
+
+        # should not use aliasing or adaption so should be cheap
+        @profiling.function_call_count(times=50)
+        def go():
+            orm_join(A, B, A.b)
+
+        go()
+
+    def test_a_to_b_aliased(self):
+        A, B = self.classes("A", "B")
+
+        a1 = aliased(A)
+
+        # uses aliasing, therefore adaption which is expensive
+        @profiling.function_call_count(times=50)
+        def go():
+            orm_join(a1, B, a1.b)
+
+        go()
+
+    def test_a_to_d(self):
+        A, D = self.classes("A", "D")
+
+        # the join condition between A and D uses a secondary selectable  with
+        # overlap so incurs aliasing, which is expensive, there is also a check
+        # that determines that this overlap exists which is not currently
+        # cached
+        @profiling.function_call_count(times=50)
+        def go():
+            orm_join(A, D, A.d)
+
+        go()
+
+    def test_a_to_d_aliased(self):
+        A, D = self.classes("A", "D")
+
+        a1 = aliased(A)
+
+        # aliased, uses adaption therefore expensive
+        @profiling.function_call_count(times=50)
+        def go():
+            orm_join(a1, D, a1.d)
+
+        go()
+
+
 class BranchedOptionTest(fixtures.MappedTest):
     @classmethod
     def define_tables(cls, metadata):

diff --git a/test/ext/test_serializer.py b/test/ext/test_serializer.py
index e5490146c7ddcb23c6c6a2fa6b2a651638768d4c..e6c7b717f9ddbb13f31a726f164e43e7062e9a69 100644 (file)
--- a/test/ext/test_serializer.py
+++ b/test/ext/test_serializer.py
@@ -243,7 +243,6 @@ class SerializeTest(AssertsCompiledSQL, fixtures.MappedTest):
         j2 = serializer.loads(serializer.dumps(j, -1), users.metadata)
         assert j2.left is j.left
         assert j2.right is j.right
-        assert j2._target_adapter._next
 
     @testing.exclude(
         "sqlite", "<=", (3, 5, 9), "id comparison failing on the buildbot"

diff --git a/test/profiles.txt b/test/profiles.txt
index 493f2769833ffbe7324ed016453a9108e7bde1c7..89c01f5d1b3693e5ed0b7f846ea5ad99413bb809 100644 (file)
--- a/test/profiles.txt
+++ b/test/profiles.txt
@@ -543,6 +543,22 @@ test.aaa_profiling.test_orm.DeferOptionsTest.test_defer_many_cols 3.7_postgresql
 test.aaa_profiling.test_orm.DeferOptionsTest.test_defer_many_cols 3.7_sqlite_pysqlite_dbapiunicode_cextensions 23259
 test.aaa_profiling.test_orm.DeferOptionsTest.test_defer_many_cols 3.7_sqlite_pysqlite_dbapiunicode_nocextensions 30268
 
+# TEST: test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_b_aliased
+
+test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_b_aliased 3.7_sqlite_pysqlite_dbapiunicode_nocextensions 10313
+
+# TEST: test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_b_plain
+
+test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_b_plain 3.7_sqlite_pysqlite_dbapiunicode_nocextensions 3256
+
+# TEST: test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_d
+
+test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_d 3.7_sqlite_pysqlite_dbapiunicode_nocextensions 106798
+
+# TEST: test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_d_aliased
+
+test.aaa_profiling.test_orm.JoinConditionTest.test_a_to_d_aliased 3.7_sqlite_pysqlite_dbapiunicode_nocextensions 104564
+
 # TEST: test.aaa_profiling.test_orm.JoinedEagerLoadTest.test_build_query
 
 test.aaa_profiling.test_orm.JoinedEagerLoadTest.test_build_query 2.7_mssql_pyodbc_dbapiunicode_nocextensions 439163