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
(cherry picked from commit 7e9f273835ac68df894568ba4292bfbc74ce187b)

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 47d93e829aa6c859eec34cd461fdf337c5d4fa4f..d31df065408d614182b92f8faf0419d872cd3b3d 100644 (file)
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -1056,6 +1056,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
@@ -2228,12 +2229,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:
@@ -2242,13 +2249,20 @@ class RelationshipProperty(StrategizedProperty):
             if self._is_self_referential and source_selectable is None:
                 dest_selectable = dest_selectable.alias()
                 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_from_container  # e.g an alias
+            )
+        )
 
         (
             primaryjoin,

diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py
index 18d693279d460c25fc8054ab6abd1a9730747d64..ba07223e421ea7ce574233d9a3d52486abd8d52c 100644 (file)
--- a/lib/sqlalchemy/orm/util.py
+++ b/lib/sqlalchemy/orm/util.py
@@ -1020,6 +1020,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 7ec39ffc9efa5afbbeca8f934aa8baaf2598b92c..9c22952252f6779842081300eed2e2cfdc937053 100644 (file)
--- a/lib/sqlalchemy/testing/profiling.py
+++ b/lib/sqlalchemy/testing/profiling.py
@@ -187,7 +187,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
@@ -200,8 +200,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 4e52a777895c505a1b1c48b29cd85c01deb4d20a..ade4cb92f2fa9ad533749503b463ec3585605653 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
@@ -814,6 +818,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 67f7c337fa940b429dc4a84121edeb007022bd74..db83e90f4783cbb011eda2e5e4620afa7bc6605a 100644 (file)
--- a/test/profiles.txt
+++ b/test/profiles.txt
@@ -358,6 +358,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 24244
 test.aaa_profiling.test_orm.DeferOptionsTest.test_defer_many_cols 3.7_sqlite_pysqlite_dbapiunicode_nocextensions 27251
 
+# 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_cextensions 418033