From: Mike Bayer Date: Wed, 16 Mar 2022 16:37:20 +0000 (-0400) Subject: remove intermediary _is_clone_of entries when cloning X-Git-Tag: rel_2_0_0b1~422^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=58666d32742dc050c2e48c48ab8f946561636e8b;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git remove intermediary _is_clone_of entries when cloning Improvements in memory usage by the ORM, removing a significant set of intermediary expression objects that are typically stored when a copy of an expression object is created. These clones have been greatly reduced, reducing the number of total expression objects stored in memory by ORM mappings by about 30%. note this change causes the tests to have a bit of a harder time with GC, which we would assume is because mappings now have a lot more garbage to clean up after mappers are configured. it remains to be seen what the long term effects of this are. Fixes: #7823 Change-Id: If8729747ffb9bf27e8974f069a994b5a823ee095 --- diff --git a/doc/build/changelog/unreleased_14/7823.rst b/doc/build/changelog/unreleased_14/7823.rst new file mode 100644 index 0000000000..249a749d02 --- /dev/null +++ b/doc/build/changelog/unreleased_14/7823.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, orm, performance + :tickets: 7823 + + Improvements in memory usage by the ORM, removing a significant set of + intermediary expression objects that are typically stored when a copy of an + expression object is created. These clones have been greatly reduced, + reducing the number of total expression objects stored in memory by + ORM mappings by about 30%. diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py index fdb3fc8bbf..346a84ddfa 100644 --- a/lib/sqlalchemy/sql/elements.py +++ b/lib/sqlalchemy/sql/elements.py @@ -357,7 +357,8 @@ class ClauseElement( # process leaves around a lot of remnants of the previous clause # typically in the form of column expressions still attached to the # old table. - c._is_clone_of = self + cc = self._is_clone_of + c._is_clone_of = cc if cc is not None else self return c diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index 0692483e9f..e143d14761 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -4135,7 +4135,8 @@ class _MemoizedSelectEntities( def _clone(self, **kw): c = self.__class__.__new__(self.__class__) c.__dict__ = {k: v for k, v in self.__dict__.items()} - c._is_clone_of = self + + c._is_clone_of = self.__dict__.get("_is_clone_of", self) return c @classmethod diff --git a/test/aaa_profiling/test_memusage.py b/test/aaa_profiling/test_memusage.py index cf89dd6e2b..f084eac2c0 100644 --- a/test/aaa_profiling/test_memusage.py +++ b/test/aaa_profiling/test_memusage.py @@ -346,6 +346,19 @@ class MemUsageTest(EnsureZeroed): go() + def test_clone_expression(self): + + root_expr = column("x", Integer) == 12 + expr = root_expr + + @profile_memory() + def go(): + nonlocal expr + + expr = cloned_traverse(expr, {}, {}) + + go() + @testing.add_to_marker.memory_intensive class MemUsageWBackendTest(fixtures.MappedTest, EnsureZeroed): diff --git a/test/orm/test_hasparent.py b/test/orm/test_hasparent.py index 425dd947d4..8f61c11970 100644 --- a/test/orm/test_hasparent.py +++ b/test/orm/test_hasparent.py @@ -1,5 +1,4 @@ """test the current state of the hasparent() flag.""" - from sqlalchemy import ForeignKey from sqlalchemy import Integer from sqlalchemy import testing @@ -26,6 +25,10 @@ class ParentRemovalTest(fixtures.MappedTest): run_inserts = None + # trying to push GC to do a better job + run_setup_classes = "each" + run_setup_mappers = "each" + @classmethod def define_tables(cls, metadata): if testing.against("oracle"): @@ -173,12 +176,24 @@ class ParentRemovalTest(fixtures.MappedTest): """ User = self.classes.User s, u1, a1 = self._fixture() + gc_collect() u2 = User(addresses=[a1]) # noqa s.expire(a1) u1.addresses.remove(a1) + u2_is = u2._sa_instance_state + del u2 + + for i in range(5): + gc_collect() + # heisenberg the GC a little bit, since #7823 caused a lot more + # GC when mappings are set up, larger test suite started failing + # on this being gc'ed + o = u2_is.obj() + assert o is None + # controversy here. The action is # to expire one object, not the other, and remove; # this is pretty abusive in any case. for now @@ -192,13 +207,23 @@ class ParentRemovalTest(fixtures.MappedTest): def test_stale_state_negative(self): User = self.classes.User s, u1, a1 = self._fixture() + gc_collect() u2 = User(addresses=[a1]) s.add(u2) s.flush() s._expunge_states([attributes.instance_state(u2)]) + + u2_is = u2._sa_instance_state del u2 - gc_collect() + + for i in range(5): + gc_collect() + # heisenberg the GC a little bit, since #7823 caused a lot more + # GC when mappings are set up, larger test suite started failing + # on this being gc'ed + o = u2_is.obj() + assert o is None assert_raises_message( orm_exc.StaleDataError, diff --git a/test/orm/test_session.py b/test/orm/test_session.py index d146612a5a..56371bc68c 100644 --- a/test/orm/test_session.py +++ b/test/orm/test_session.py @@ -1568,14 +1568,22 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): s = fixture_session() self.mapper_registry.map_imperatively(User, users) + gc_collect() s.add(User(name="ed")) s.flush() assert not s.dirty user = s.query(User).one() + + # heisenberg the GC a little bit, since #7823 caused a lot more + # GC when mappings are set up, larger test suite started failing + # on this being gc'ed + user_is = user._sa_instance_state del user gc_collect() + assert user_is.obj() is None + assert len(s.identity_map) == 0 user = s.query(User).one() @@ -1600,6 +1608,7 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): s = fixture_session() self.mapper_registry.map_imperatively(User, users) + gc_collect() s.add(User(name="ed")) s.flush() @@ -1642,6 +1651,8 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): properties={"addresses": relationship(Address, backref="user")}, ) self.mapper_registry.map_imperatively(Address, addresses) + gc_collect() + s.add(User(name="ed", addresses=[Address(email_address="ed1")])) s.commit() @@ -1682,6 +1693,8 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): }, ) self.mapper_registry.map_imperatively(Address, addresses) + gc_collect() + s.add(User(name="ed", address=Address(email_address="ed1"))) s.commit() @@ -1709,6 +1722,7 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): users, User = self.tables.users, self.classes.User self.mapper_registry.map_imperatively(User, users) + gc_collect() sess = Session(testing.db) @@ -1740,6 +1754,7 @@ class WeakIdentityMapTest(_fixtures.FixtureTest): users, User = self.tables.users, self.classes.User self.mapper_registry.map_imperatively(User, users) + gc_collect() sess = fixture_session()