On Jan 11, 2008, at 9:24 AM, Jim Fulton wrote:
While messing with Zope2's webdav implementation, I ran across this
bit of memory-management code:
dflag = hasattr(ob, '_p_changed') and (ob._p_changed == None)
... stuff ...
I actually think this should be:
dflag = not getattr(ob, '_p_changed', None)
... stuff ...
.. because when _p_changed on a persistent object (as I read it in
the persistence interface file) is None, the object is a ghost.
The object will never be a ghost here because non-_p_ attributes
are looked up on it before we check for _p_changed, and even if it
was a ghost, deactivating a ghost (to turn it into a ghost) is just
not useful. I think the only time we don't want to deactivate it
is if it has been changed (when _p_changed is True). Or at least
that seems to be the intent.
Does this sound right?
The important thing to note above is that the check is made *before*
".. stuff ..". The idea is that we want to free any objects we
loaded while doing whatever we are doing. We check if an object is
a ghost before doing something and then, if it was a ghost before,
we try to make it a ghost when we're done. We try to leave the
object as we found it. With your change, we'd be ghostifying
objects that were presumably useful, because they were in the cache,
and we'd be failing to ghostify objects that we resurrected just for
I see. The earliest place I've seen this code is almost ten years
old, it's amazing you remember. The intent was misunderstood over the
years, because code like this got into various places:
for ob in obj.listDAVObjects():
if ob.meta_type=="Broken Because Product is Gone":
dflag=hasattr(ob, '_p_changed') and (ob._p_changed ==
the (ob._p_changed == None) will never be true here as we will have
woken it up by looking for 'meta_type'. This is what got me confused
With your change, we'd be ghostifying objects that were presumably
useful, because they were in the cache, and we'd be failing to
ghostify objects that we resurrected just for the operation.
The former is true, the latter no. Objects that are resurrected just
for the operation would be ghostified too. Only objects that were
_p_changed = True would not be. But it doesn't matter, I just needed
to understand the intent, and now I do. I don't intend to check my
competing example code in, I'll just fix the code around the places
where the pattern was misunderstood.
Related.... but fuzzier... is it your expectation that the amount of
memory used for a database walk routine that tries to do memory
management via some combination of connection.cacheMinimize-or-
cacheGC() every-n-iterations (no calls to individual objects'
_p_deactivate) should be close to one which uses _p_deactivate
aggressively against the objects being walked? In my experience, no
combination of the "aggregate" cache management calls seems to work
nearly as well as aggressively _p_deactivate-ing walked objects
directly while walking a large object tree (at least under ZODB 3.6).
It seems like doing cacheMinimize, etc just doesn't really have much
effect on real memory usage during the walk when it's the only thing
used. It's a difficult thing to test, as you need a truly huge
database to finaly see the failure mode (which is that you run out of
RAM ;-), but that's my experience anyway.
Zope-Dev maillist - Zope-Dev@zope.org
** No cross posts or HTML encoding! **
(Related lists -