On Tuesday April 26 2011 01:01:55 Martin Sandve Alnæs wrote: > I'm not sure if this is safe or even solves anything. > If there are circular references a.b = b; b.a = a, > a.__del__ won't be called if the reference from > b.a is still intact, since a.__del__ is called when > the reference count of a goes to 0. Adding the > __del__ function will also stop the cyclic reference > detection in Python from cleaning up. That is my > understanding after reading > http://docs.python.org/reference/datamodel.html > just now. Correct me if I'm wrong!
If the above situation was correct you would be right. As it is now a preprocessed form keeps a reference to form_data and form_data keeps a reference to the preprocessed form => Ciruclar dependency. I tried to break that by deleting the preprocessed form from its form_data, when the original form is deleted (not the preprocessed one.) When this happens the preprocessed form will be deleted (I guess). Not sure why the preprocessed form need to keep a reference to the form_data, though. Removing that reference would solve the circular dependency. > I don't know which memory cache you're referring to here. > Instant has a memory cache to avoid unnecessary disk access. The memory cache refered to here was the one I removed. It was a memory cache which cached the preprocessed form, using the id() of the original form. I think this was the initial issue, as the id of a form turned out to be not safe. But then it also turned out to be superflous as the preprocessed form were attached to the form_data anyhow. So the issue is essentially solved. We just need to get rid of the ciruclar dependency. Which I now think we can do by removing the reference to the preprocessed form from form_data. > And this patch does not seem related to any of > "the issues" I've seen mentioned in this thread. > Cyclic Python object references causing memory > leaks sounds like a potential important issue, but > that's not what this thread is about is it? No. It was just an issue I stumbled across while trying to fix the really issue. Sorry for not being clear on that. Johan > Martin > > On 25 April 2011 23:50, Johan Hake <johan.h...@gmail.com> wrote: > > This should be fixed now. > > > > I do not see why we introduced the memory cache when this solution was > > laying > > right in front our eyes... > > > > Anyhow. Here is a patch for ufl to avoid circular dependency between a > > preprocessed form and the form_data. > > > > Johan > > > > On Monday April 25 2011 14:34:00 Anders Logg wrote: > > > Simple sounds good. > > > > > > -- > > > Anders > > > > > > On Mon, Apr 25, 2011 at 02:29:50PM -0700, Johan Hake wrote: > > > > I am working on a simple solution, where we store everything in the > > > > original ufl form. > > > > > > > > I might have something soon. > > > > > > > > Johan > > > > > > > > On Monday April 25 2011 14:26:18 Garth N. Wells wrote: > > > > > On 25/04/11 22:08, Anders Logg wrote: > > > > > > On Mon, Apr 25, 2011 at 07:40:21PM -0000, Garth Wells wrote: > > > > > >> On 25/04/11 20:00, Johan Hake wrote: > > > > > >>> On Monday April 25 2011 11:26:36 Garth Wells wrote: > > > > > >>>> On 25/04/11 18:51, Anders Logg wrote: > > > > > >>>>> On Mon, Apr 25, 2011 at 05:11:41PM -0000, Garth Wells wrote: > > > > > >>>>>> On 25/04/11 17:53, Johan Hake wrote: > > > > > >>>>>>> On Monday April 25 2011 08:59:18 Garth Wells wrote: > > > > > >>>>>>>> On 25/04/11 16:47, Johan Hake wrote: > > > > > >>>>>>>>> Commenting out the cache is really not a fix. The problem > > > > is > > > > > > > >>>>>>>>> within dolfin. Isn't there another way to deal with this? > > > > > >>>>>>>> > > > > > >>>>>>>> It is a fix if the cache isn't needed. > > > > > >>>>>>> > > > > > >>>>>>> Sure. > > > > > >>>>>>> > > > > > >>>>>>>>> First: How much penalty are there with a disabled memory > > > > > >>>>>>>>> cache. Maybe the problem isn't that bad? > > > > > >>>>>>>> > > > > > >>>>>>>> I don't get the point of this cache. The way it is now, a > > > > form > > > > > > > >>>>>>>> is only preprocessed if it hasn't already been > > > > > >>>>>>>> preprocessed, which seems ok to me. The old code tried to > > > > > >>>>>>>> avoid some preprocessing, but it was highly dubious and I > > > > > >>>>>>>> doubt that it was effective. > > > > > >>>>>>> > > > > > >>>>>>> I think the preprocessing stage actually do take some time. > > > > > >>>>>>> AFAIK the preproces stage essentially do two things. It > > > > > >>>>>>> creates a canonical version of the Form so two Forms that > > > > > >>>>>>> are the same, but constructed at different times are > > > > > >>>>>>> beeing treated equal wrt form generation. Then are DOLFIN > > > > > >>>>>>> specific guys extracted. I am not sure what takes the most > > > > > >>>>>>> time. We should probably profiel it... But if it is the > > > > > >>>>>>> latter we > > > > could > > > > > > > >>>>>>> consider putting another cache in place which is more > > > > > >>>>>>> robust wrt changing DOLFIN objects. > > > > > >>>>>> > > > > > >>>>>> It should be easy to avoid the overhead of preprocessing by > > > > > >>>>>> keeping the object in scope. If the object changes, the only > > > > > >>>>>> robust way to make sure that the form is the same as one in > > > > the > > > > > > > >>>>>> cache is to compare all the data. This requires > > > > > >>>>>> preprocessing the form, which then defeats the purpose of a > > > > > >>>>>> cache. It may be possible to add a lightweight preprocess > > > > > >>>>>> to UFL, but I don't think that it's worth the effort or > > > > > >>>>>> extra complication. > > > > > >>> > > > > > >>> I think a light weight version might be the way to go. This is > > > > then > > > > > > > >>> stored in memory cache. If we are able to strip such a form for > > > > all > > > > > > > >>> DOLFIN specific things we would also prevent huge memory leaks > > > > with > > > > > > > >>> mesh beeing kept. > > > > > >>> > > > > > >>> Then we always grab DOLFIN specific data from the passed form > > > > > >>> instead of grabbing from the cache. Not sure how easy this will > > > > be > > > > > > > >>> to implement, but I think we need to explore it, as the DOLFIN > > > > > >>> specific part of the form really has nothing to do with the > > > > > >>> generated Form. > > > > > >>> > > > > > >>> Martin: > > > > > >>> Why is it important to have the _count in the repr of the form? > > > > > >>> I guess that is used in ufl algorithms? Would it be possible > > > > > >>> to include a second repr function, which did not include the > > > > > >>> count? This would then be used when the signature is checked > > > > > >>> for. We could then use that repr to generate a form which is > > > > > >>> stored in > > > > the > > > > > > > >>> memory cache. This would then be tripped for any DOLFIN > > > > > >>> specific objects. This should work as the _count attribute has > > > > > >>> nothing to do with what code gets generated, but it is > > > > > >>> essential for > > > > internal > > > > > > > >>> UFL algorithms, right? > > > > > >>> > > > > > >>>>> I'm not very happy with this change. > > > > > >>>> > > > > > >>>> The bright side is that slow and correct is a better starting > > > > > >>>> point than fast but wrong ;). > > > > > >>>> > > > > > >>>> An easy fix is to attach the preprocessed form to a Form > > > > > >>>> object. This would work robustly if we can make forms > > > > > >>>> immutable once they've been compiled. Is it possible to make > > > > > >>>> a Python object immutable? > > > > > >>> > > > > > >>> We can probably overload all setattribtue methods which > > > > > >>> prohibits > > > > a > > > > > > > >>> user to write to these but it might not be possible to prohibit > > > > > >>> a user to change attributes on instances owned by the Form. I > > > > > >>> guess this is similare to the difficulties of preserving > > > > > >>> constness in C++, but I think it is even harder in Python. > > > > > >> > > > > > >> What if we have the FFC jit compiler return the preprocessed > > > > > >> form, and inside dolfin.Form simply do > > > > > >> > > > > > >> class Form(cpp.Form): > > > > > >> def __init__(self, form, . . .. ) > > > > > >> .... > > > > > >> > > > > > >> (...., preprocessed_form) = jit(form, . . . . ) > > > > > >> > > > > > >> form = preprocessed_form > > > > > >> > > > > > >> ..... > > > > > >> > > > > > >> This way, form will have form_data, and the FFC jit function > > > > > >> will know not to call ufl.preprocess. > > > > > > > > > > > > Here's another strange thing. In the JITObject class, we have two > > > > > > functions: __hash__ and signature. As far as I understand, the > > > > first > > > > > > > > is used to located objects (generated code/modules) in the > > > > > > Instant in-memory cache, while the second is used for the > > > > > > on-disk cache. > > > > > > > > > > > >>From some simple tests I did now, it looks like the __hash__ > > > > function > > > > > > > > does not need to any significant speedup. The JIT benchmark runs > > > > just > > > > > > > > as fast if I call signature from within __hash__. > > > > > > > > > > > > Furthermore, the __hash__ function must also be broken since it > > > > > > relies on calling id on the form. > > > > > > > > > > > > Ideally, we should get Instant to handle the caching, both > > > > in-memory > > > > > > > > and on-disk, by providing two functions __hash__ (fast, for > > > > in-memory > > > > > > > > cache) and signature (slow, for on-disk cache). > > > > > > > > > > > > Since __hash__ cannot call id, it must be able to attach a unique > > > > > > string to the form (perhaps based on an internal counter in FFC). > > > > > > My suggestion would be to add this to UFL, something like > > > > > > set_hash and hash (which would return None if set_hash has not > > > > > > been called). If Martin does not like that, we should be able to > > > > > > handle it on the DOLFIN side. > > > > > > > > > > > > So in conclusion: no in-memory cache in FFC (handled by Instant) > > > > and > > > > > > > > FFC attaches a hash to incoming forms so that Instant may > > > > > > recognize them later. > > > > > > > > > > The code that I disabled was caching preprocessed forms, so I don't > > > > see > > > > > > > how this can be handled by Instant. > > > > > > > > > > Garth > > > > > > > > > > > Maybe even better: Instant checks whether an incoming object has > > > > > > a set_hash function and if so calls it so it can recognize > > > > > > objects it sees a second time. > > > > > > > > > > > > I'm moving this discussion to the mailing list(s). > > > > > > > > > > _______________________________________________ > > > > > Mailing list: https://launchpad.net/~ufl > > > > > Post to : u...@lists.launchpad.net > > > > > Unsubscribe : https://launchpad.net/~ufl > > > > > More help : https://help.launchpad.net/ListHelp > > > > _______________________________________________ > > Mailing list: https://launchpad.net/~ufl > > Post to : u...@lists.launchpad.net > > Unsubscribe : https://launchpad.net/~ufl > > More help : https://help.launchpad.net/ListHelp _______________________________________________ Mailing list: https://launchpad.net/~ffc Post to : ffc@lists.launchpad.net Unsubscribe : https://launchpad.net/~ffc More help : https://help.launchpad.net/ListHelp
