{"action":"created","issue":{"url":"https://api.github.com/repos/zopefoundation/persistent/issues/44","repository_url":"https://api.github.com/repos/zopefoundation/persistent","labels_url":"https://api.github.com/repos/zopefoundation/persistent/issues/44/labels{/name}","comments_url":"https://api.github.com/repos/zopefoundation/persistent/issues/44/comments","events_url":"https://api.github.com/repos/zopefoundation/persistent/issues/44/events","html_url":"https://github.com/zopefoundation/persistent/pull/44","id":166914288,"number":44,"title":"On deactivate release in-slots objects too","user":{"login":"navytux","id":1794248,"avatar_url":"https://avatars.githubusercontent.com/u/1794248?v=3","gravatar_id":"","url":"https://api.github.com/users/navytux","html_url":"https://github.com/navytux","followers_url":"https://api.github.com/users/navytux/followers","following_url":"https://api.github.com/users/navytux/following{/other_user}","gists_url":"https://api.github.com/users/navytux/gists{/gist_id}","starred_url":"https://api.github.com/users/navytux/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/navytux/subscriptions","organizations_url":"https://api.github.com/users/navytux/orgs","repos_url":"https://api.github.com/users/navytux/repos","events_url":"https://api.github.com/users/navytux/events{/privacy}","received_events_url":"https://api.github.com/users/navytux/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":16,"created_at":"2016-07-21T20:48:48Z","updated_at":"2016-10-28T17:47:39Z","closed_at":"2016-08-10T14:41:54Z","pull_request":{"url":"https://api.github.com/repos/zopefoundation/persistent/pulls/44","html_url":"https://github.com/zopefoundation/persistent/pull/44","diff_url":"https://github.com/zopefoundation/persistent/pull/44.diff","patch_url":"https://github.com/zopefoundation/persistent/pull/44.patch"},"body":"On ._p_deactivate() and ._p_invalidate(), when an object goes to ghost\r\nstate, objects referenced by all its attributes, except related to\r\npersistence machinery, are released, this way freeing memory (if they\r\nwere referenced only from going-to-ghost object).\r\n\r\nThat's the idea - an object in ghost state is simply a stub, which loads\r\nits content on first access (via hooking into get/set attr) while\r\noccupying minimal memory in not-yet-loaded state.\r\n\r\nHowever the above is not completely true right now, as currently on\r\nghostification only object's .__dict__ is released, while in-slots objects\r\nare retained attached to ghost object staying in RAM:\r\n\r\n    ---- 8< ----\r\n    from ZODB import DB\r\n    from persistent import Persistent\r\n    import gc\r\n\r\n    db = DB(None)\r\n    jar = db.open()\r\n\r\n    class C:\r\n        def __init__(self, v):\r\n            self.v = v\r\n        def __del__(self):\r\n            print 'released (%s)' % self.v\r\n\r\n    class P1(Persistent):\r\n        pass\r\n\r\n    class P2(Persistent):\r\n        __slots__ = ('aaa')\r\n\r\n    p1 = P1()\r\n    jar.add(p1)\r\n    p1.aaa = C(1)\r\n\r\n    p2 = P2()\r\n    jar.add(p2)\r\n    p2.aaa = C(2)\r\n\r\n    p1._p_invalidate()\r\n    # \"released (1)\" is printed\r\n\r\n    p2._p_invalidate()\r\n    gc.collect()\r\n    # \"released (2)\" is NOT printed     <--\r\n    ---- 8< ----\r\n\r\nSo teach ghostify() & friends to release objects in slots to free-up\r\nmemory when an object goes to ghost state.\r\n\r\nNOTE PyErr_Occurred() added after ghostify() calls because\r\npickle_slotnames() can raise an error, but we do not want to change\r\nghostify() prototype for backward compatibility reason - as it is used\r\nin cPersistenceCAPIstruct.\r\n\r\n( I hit this bug with wendelin.core which uses proxies to load\r\n  data from DB to virtual memory manager and then deactivate proxy right\r\n  after load has been completed:\r\n\r\n  https://lab.nexedi.com/nexedi/wendelin.core/blob/f7803634/bigfile/file_zodb.py#L239\r\n  https://lab.nexedi.com/nexedi/wendelin.core/blob/f7803634/bigfile/file_zodb.py#L295 )\r\n\r\n/cc @jimfulton, @jmuchemb "},"comment":{"url":"https://api.github.com/repos/zopefoundation/persistent/issues/comments/256984250","html_url":"https://github.com/zopefoundation/persistent/pull/44#issuecomment-256984250","issue_url":"https://api.github.com/repos/zopefoundation/persistent/issues/44","id":256984250,"user":{"login":"jamadden","id":1256082,"avatar_url":"https://avatars.githubusercontent.com/u/1256082?v=3","gravatar_id":"","url":"https://api.github.com/users/jamadden","html_url":"https://github.com/jamadden","followers_url":"https://api.github.com/users/jamadden/followers","following_url":"https://api.github.com/users/jamadden/following{/other_user}","gists_url":"https://api.github.com/users/jamadden/gists{/gist_id}","starred_url":"https://api.github.com/users/jamadden/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamadden/subscriptions","organizations_url":"https://api.github.com/users/jamadden/orgs","repos_url":"https://api.github.com/users/jamadden/repos","events_url":"https://api.github.com/users/jamadden/events{/privacy}","received_events_url":"https://api.github.com/users/jamadden/received_events","type":"User","site_admin":false},"created_at":"2016-10-28T17:47:39Z","updated_at":"2016-10-28T17:47:39Z","body":"I took a stab at fixing this in issue #49 and the branch https://github.com/zopefoundation/persistent/compare/new_ghost_doesnt_clear_newargs \r\n\r\nUnfortunately, it doesn't seem to be fixable in pure python at this level.\r\n\r\nIn the case of a proxy, in C, the wrapped object is not available in the `__dict__` or `__slots__` *at all*, it's just in the struct, so it never gets cleared. In Python, we have no choice but to store it somewhere (and a slot is the best choice for it). But if it's only set by `__new__` then once `_p_deactivate` has been called, the object cannot be properly unghosted anymore.\r\n\r\nHere's that same test case from above, with some inline notes. My branch gets it farther, but once we start clearing slots, it still fails:\r\n```python\r\n >>> p = ContainedProxy([1])\r\n >>> p.__parent__ = 2\r\n >>> p.__name__ = 'test'\r\n >>> db = DB(DemoStorage('test_storage'))\r\n >>> c = db.open()\r\n >>> c.root()['p'] = p\r\n >>> transaction.commit()\r\n\r\n >>> c2 = db.open()\r\n >>> p2 = c2.root()['p']\r\n >>> p2\r\n [1] # Previously, we blew up here because _wrapped was gone. My branch fixes this.\r\n >>> p2.__parent__\r\n 2\r\n >>> p2.__name__\r\n 'test'\r\n\r\n >>> p2._p_changed\r\n 0\r\n >>> p2._p_deactivate()\r\n >>> bool(p2._p_changed)\r\n False\r\n >>> p2.__name__\r\n 'test'\r\n >>> p2\r\n [1] # Now we blow up here. Once _p_deactivate has been called, we have no way to reconstitute _wrapped\r\n```\r\n\r\nHere are our options as I see them:\r\n\r\n1. Stick with the status quo: don't clear `__slots__`.\r\n2. Require every subclass who uses `__new__` and `__reduce__` or `__getnewargs__` to *also* start overriding `_p_deactivate` and only clear things that they can reconstitute from `__setstate__`.\r\n\r\nBreaking pickle compatibility might be an option in some cases to store more information in the value passed to `__setstate__`, but it's not an appealing one.\r\n\r\nUsing `__new__` and `__reduce__`/`__getnewargs__` are relatively advanced things to do, but they're within the realm of standard Python and standard pickle. People can be expected to be using them. But overriding `_p_deactivate` is persistent-specific and even more advanced (and discouraged?).\r\n\r\nHave I missed anything? \r\n\r\nMy vote at this point is leaning towards the status quo and not clearing `__slots__`."}}
{"actor":{"display_login":"jamadden"}}