From abcb5605f91ef206dd5f0f6400302f0b28425365 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sun, 19 Oct 2008 19:26:48 +0000 Subject: [PATCH] - Improved weakref identity map memory management to no longer require mutexing, resurrects garbage collected instance on a lazy basis for an InstanceState with pending changes. --- CHANGES | 4 ++ lib/sqlalchemy/orm/attributes.py | 3 +- lib/sqlalchemy/orm/identity.py | 89 +++++++++++++++++--------------- lib/sqlalchemy/orm/session.py | 13 ++--- test/orm/session.py | 2 +- test/orm/transaction.py | 32 +++++++++++- test/profiling/zoomark_orm.py | 4 +- 7 files changed, 88 insertions(+), 59 deletions(-) diff --git a/CHANGES b/CHANGES index 62b3ca77e6..60716cc3c9 100644 --- a/CHANGES +++ b/CHANGES @@ -14,6 +14,10 @@ CHANGES a collection to an iterable. Use contains() to test for collection membership. + - Improved weakref identity map memory management to no longer + require mutexing, resurrects garbage collected instance + on a lazy basis for an InstanceState with pending changes. + - sql - Further simplified SELECT compilation and its relationship to result row processing. diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index 79e5d22f7b..0424d9b7c6 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -796,7 +796,8 @@ class InstanceState(object): self.expired = False def dispose(self): - del self.session_id + if self.session_id: + del self.session_id @property def sort_key(self): diff --git a/lib/sqlalchemy/orm/identity.py b/lib/sqlalchemy/orm/identity.py index 4232821612..8ad2b64bb2 100644 --- a/lib/sqlalchemy/orm/identity.py +++ b/lib/sqlalchemy/orm/identity.py @@ -75,15 +75,12 @@ class WeakInstanceDict(IdentityMap): def __init__(self): IdentityMap.__init__(self) self._wr = weakref.ref(self) - # RLock because the mutex is used by a cleanup - # handler, which can be called at any time (including within an already mutexed block) - self._mutex = base_util.threading.RLock() def __getitem__(self, key): state = dict.__getitem__(self, key) o = state.obj() if o is None: - o = state._check_resurrect(self) + o = state._is_really_none() if o is None: raise KeyError, key return o @@ -93,7 +90,7 @@ class WeakInstanceDict(IdentityMap): state = dict.__getitem__(self, key) o = state.obj() if o is None: - o = state._check_resurrect(self) + o = state._is_really_none() except KeyError: return False return o is not None @@ -198,52 +195,58 @@ class IdentityManagedState(attributes.InstanceState): def _instance_dict(self): return None - def _check_resurrect(self, instance_dict): - instance_dict._mutex.acquire() - try: - return self.obj() or self.__resurrect(instance_dict) - finally: - instance_dict._mutex.release() - def modified_event(self, attr, should_copy, previous, passive=False): attributes.InstanceState.modified_event(self, attr, should_copy, previous, passive) instance_dict = self._instance_dict() if instance_dict: instance_dict.modified = True + + def _is_really_none(self): + """do a check modified/resurrect. + This would be called in the extremely rare + race condition that the weakref returned None but + the cleanup handler had not yet established the + __resurrect callable as its replacement. + + """ + if self.check_modified(): + self.obj = self.__resurrect + return self.obj() + else: + return None + def _cleanup(self, ref): - # tiptoe around Python GC unpredictableness - try: - instance_dict = self._instance_dict() - instance_dict._mutex.acquire() - except: - return - # the mutexing here is based on the assumption that gc.collect() - # may be firing off cleanup handlers in a different thread than that - # which is normally operating upon the instance dict. - try: - try: - self.__resurrect(instance_dict) - except: - # catch app cleanup exceptions. no other way around this - # without warnings being produced - pass - finally: - instance_dict._mutex.release() - - def __resurrect(self, instance_dict): + """weakref callback. + + This method may be called by an asynchronous + gc. + + If the state shows pending changes, the weakref + is replaced by the __resurrect callable which will + re-establish an object reference on next access, + else removes this InstanceState from the owning + identity map, if any. + + """ if self.check_modified(): - # store strong ref'ed version of the object; will revert - # to weakref when changes are persisted - obj = self.manager.new_instance(state=self) - self.obj = weakref.ref(obj, self._cleanup) - self._strong_obj = obj - obj.__dict__.update(self.dict) - self.dict = obj.__dict__ - self._run_on_load(obj) - return obj + self.obj = self.__resurrect else: - instance_dict.remove(self) + instance_dict = self._instance_dict() + if instance_dict: + instance_dict.remove(self) self.dispose() - return None + + def __resurrect(self): + """A substitute for the obj() weakref function which resurrects.""" + + # store strong ref'ed version of the object; will revert + # to weakref when changes are persisted + obj = self.manager.new_instance(state=self) + self.obj = weakref.ref(obj, self._cleanup) + self._strong_obj = obj + obj.__dict__.update(self.dict) + self.dict = obj.__dict__ + self._run_on_load(obj) + return obj diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index 387f5a28d6..9eee0e3e13 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -787,13 +787,7 @@ class Session(object): """ for state in self.identity_map.all_states() + list(self._new): - try: - del state.session_id - except AttributeError: - # asynchronous GC can sometimes result - # in the auto-disposal of an InstanceState within this process - # see test/orm/session.py DisposedStates - pass + state.dispose() self.identity_map = self._identity_cls() self._new = {} @@ -1018,11 +1012,11 @@ class Session(object): def _expunge_state(self, state): if state in self._new: self._new.pop(state) - del state.session_id + state.dispose() elif self.identity_map.contains_state(state): self.identity_map.discard(state) self._deleted.pop(state, None) - del state.session_id + state.dispose() def _register_newly_persistent(self, state): mapper = _state_mapper(state) @@ -1057,7 +1051,6 @@ class Session(object): self.identity_map.discard(state) self._deleted.pop(state, None) - del state.session_id @util.pending_deprecation('0.5.x', "Use session.add()") def save(self, instance): diff --git a/test/orm/session.py b/test/orm/session.py index 4597137939..eecb13d07f 100644 --- a/test/orm/session.py +++ b/test/orm/session.py @@ -748,7 +748,7 @@ class SessionTest(_fixtures.FixtureTest): gc.collect() assert len(s.identity_map) == 1 assert len(s.dirty) == 1 - + assert None not in s.dirty s.flush() gc.collect() assert not s.dirty diff --git a/test/orm/transaction.py b/test/orm/transaction.py index 273a35222f..5c3fd8342d 100644 --- a/test/orm/transaction.py +++ b/test/orm/transaction.py @@ -1,11 +1,12 @@ import testenv; testenv.configure_for_tests() import operator from sqlalchemy import * +from sqlalchemy.orm import attributes from sqlalchemy import exc as sa_exc from sqlalchemy.orm import * from testlib import * from testlib.fixtures import * - +import gc class TransactionTest(FixtureTest): keep_mappers = True @@ -89,7 +90,34 @@ class AutoExpireTest(TransactionTest): s.rollback() assert u1 in s assert u1 not in s.deleted - + + def test_gced_delete_on_rollback(self): + s = self.session() + u1 = User(name='ed') + s.add(u1) + s.commit() + + s.delete(u1) + u1_state = attributes.instance_state(u1) + assert u1_state in s.identity_map.all_states() + assert u1_state in s._deleted + s.flush() + assert u1_state not in s.identity_map.all_states() + assert u1_state not in s._deleted + del u1 + gc.collect() + assert u1_state.obj() is None + + s.rollback() + assert u1_state in s.identity_map.all_states() + u1 = s.query(User).filter_by(name='ed').one() + assert u1_state not in s.identity_map.all_states() + assert s.scalar(users.count()) == 1 + s.delete(u1) + s.flush() + assert s.scalar(users.count()) == 0 + s.commit() + def test_trans_deleted_cleared_on_rollback(self): s = self.session() u1 = User(name='ed') diff --git a/test/profiling/zoomark_orm.py b/test/profiling/zoomark_orm.py index 39446dbf84..1a179cc0ce 100644 --- a/test/profiling/zoomark_orm.py +++ b/test/profiling/zoomark_orm.py @@ -298,11 +298,11 @@ class ZooMarkTest(TestBase): def test_profile_2_insert(self): self.test_baseline_2_insert() - @profiling.function_call_count(8323) + @profiling.function_call_count(8097) def test_profile_3_properties(self): self.test_baseline_3_properties() - @profiling.function_call_count(29494) + @profiling.function_call_count(28211) def test_profile_4_expressions(self): self.test_baseline_4_expressions() -- 2.47.3