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

7 files changed, 140 insertions, 6 deletions

diff --git a/doc/build/changelog/unreleased_13/rel_join.rst b/doc/build/changelog/unreleased_13/rel_join.rst
new file mode 100644
index 000000000..1f0e29ef1
--- /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 fc3e78ea1..7ee12ca6c 100644
--- 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 b96387c08..2f78fa535 100644
--- 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 bfc4997e1..b837d9b5b 100644
--- 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 209bc02e3..e2df85a33 100644
--- 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 e5490146c..e6c7b717f 100644
--- 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 493f27698..89c01f5d1 100644
--- 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