From: Mike Bayer Date: Thu, 19 Dec 2019 02:50:24 +0000 (-0500) Subject: Don't apply aliasing + adaption for simple relationship joins X-Git-Tag: rel_1_3_13~27^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f6b52bef082f2b2d4e4d7360794b9b55fb01a044;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 index 0000000000..1f0e29ef13 --- /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 47d93e829a..d31df06540 100644 --- 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 18d693279d..ba07223e42 100644 --- 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 7ec39ffc9e..9c22952252 100644 --- 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 4e52a77789..ade4cb92f2 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 @@ -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 e5490146c7..e6c7b717f9 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 67f7c337fa..db83e90f47 100644 --- 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