Subject: Re: [Ufl] [Bug 769811] [NEW] JIT cache problem with id(form)

From: Anders Logg <[email-masked]>
Date: Mon, 25 Apr 2011 23:08:15 +0200

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.

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).

--
Anders

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 22:26:18 +0100

> FFC attaches a hash to incoming forms so that Instant may recognize

The code that I disabled was caching preprocessed forms, so I don't see
how this can be handled by Instant.

Garth

> _______________________________________________
> Mailing list: https://launchpad.net/~ffc
> Post to     : [email-masked]
> Unsubscribe : https://launchpad.net/~ffc
> More help   : https://help.launchpad.net/ListHelp

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 14:29:50 -0700

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:
> >>>>>>>>> Commenting out the cache is really not a fix. The problem is
> >>>>>>>>> within dolfin. Isn't there another way to deal with this?
> >>>>>>>> 
> >>>>>>>>> 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?
> >>> 
> >>>> The bright side is that slow and correct is a better starting point
> >>>> than fast but wrong ;).
> >>>> 
> >>> 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.
> >> 
> Mailing list: https://launchpad.net/~ufl
> Post to     : [email-masked]
> Unsubscribe : https://launchpad.net/~ufl

---

From: Anders Logg <[email-masked]>
Date: Mon, 25 Apr 2011 23:33:52 +0200

> how this can be handled by Instant.

The point would be that one could check that "hash" of the form (some
unique string) instead of computing the signature which involves
preprocessing the form.

It would work with Instant since Instant calls the __hash__ function
to see whether the form is in the memory cache. Only after the __hash__
function has been called does it call the signature function (if I
remember correctly).

--
Anders

---

From: Anders Logg <[email-masked]>
Date: Mon, 25 Apr 2011 23:34:00 +0200

Simple sounds good.

--
Anders

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 22:41:58 +0100

> unique string) instead of computing the signature which involves

How would the hash be computed? To check if the mesh has changed, my
limited understanding is that the entire object would have to be
serialised, and then a hash computed. How expensive is that?

The issue that I ran into was not related to signatures. It was related
to the non-UFL data that is attached to arguments.

Garth

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 22:44:23 +0100

> I might have something soon.

OK.

I played around with something like this, but had some issues when the
form changes (i.e. a += . . . ) after being preprocessed.

Garth

---

From: Anders Logg <[email-masked]>
Date: Mon, 25 Apr 2011 23:48:33 +0200

> to the non-UFL data that is attached to arguments.

The hash would be unique to each form. It could just be a counter
value and the counter would be increased inside Instant for each
object it gets as input.

We could also include the output of repr if we want, but as Martin
points out we should not modify forms so then it shouldn't be needed.
(See comment about why repr is included in jitcompiler.py.)

--
Anders

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 14:50:58 -0700

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:
> > I am working on a simple solution, where we store everything in the
> > original ufl form.
> > 
> > > >>>>>>>>> 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.
> > > >>> 
> > > >>> 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?
> > > >>> 
> > > >>>> 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?
> > > >>> 
> > > >>> 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):
> > > >> This way, form will have form_data, and the FFC jit function will
> > > >> know not to call ufl.preprocess.
> > > > 
> > > > Furthermore, the __hash__ function must also be broken since it
> > > > relies on calling id on the form.
> > > > 
# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: [email-masked]
# target_branch: bzr+ssh://bazaar.launchpad.net/~ufl-core/ufl/main/
# testament_sha1: 48930d933e8fde5c7b4b1b7b1984a67c3e4f23d8
# timestamp: 2011-04-25 14:42:54 -0700
# base_revision_id: [email-masked]
# 
# Begin patch
=== modified file 'ufl/form.py'
--- ufl/form.py	2011-04-12 13:03:47 +0000
+++ ufl/form.py	2011-04-25 21:40:56 +0000
@@ -95,6 +95,12 @@
         "Return form metadata (None if form has not been preprocessed)"
         return self._form_data
 
+    def __del__(self):
+        if self._form_data is not None:
+            # Delete preprocessed form from form_data, preventing memory
+            # leak cause by circular referencing
+            del self._form_data._form
+    
     def __add__(self, other):
         # --- Add integrands of integrals with the same measure

# Begin bundle

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 14:53:22 -0700

> form changes (i.e. a += . . . ) after being preprocessed.

Oups! I guess my "fix" wont get this either... Maybe we need to destroy the 
form_data object whenever += ... are called?

Johan

> >>>>>>>>> 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.
> >>>>>>>> 
> >>>>>>>> cache is to compare all the data. This requires preprocessing the
> >>>>>>>> form, which then defeats the purpose of a cache. It may be
> >>>>> 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?
> >>>>> 
> >>>>> this is similare to the difficulties of preserving constness in C++,
> >>>>> but I think it is even harder in Python.
> >>>>

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 22:56:25 +0100

> value and the counter would be increased inside Instant for each

But how does Instant know if a form is new? I also don't see why Instant
should need to know if the mesh associated with a form has changed, but
is for the rest the same. Wouldn't Instant need to be DOLFIN-aware?

Garth

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 00:04:43 +0200

> is for the rest the same. Wouldn't Instant need to be DOLFIN-aware?

The hash() function would play the same role as the id() function
before with the difference that we can't get the same id for a new
form as for an old form that's gone out of scope.

Instant should not need to know anything it just does this:

   check if object has a set_hash() function
   if so, calls hash() to get the hash value
     checks the cache for that hash value
   if not, assign unique value by calling set_hash on the object

We would need to make sure from the DOLFIN side that when we change a
Form, we also change the hash value (for example by setting it to
None) which would trigger the Instant disk cache.

--
Anders

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 15:04:48 -0700

> form_data object whenever += ... are called?

I do not think this will be a problem, as += here returns a new object. I am 
actually not sure how this works, but I think because __iadd__ is _not_ 
implemented in Form, Python probably turns:

  L += L 

into 

  L = L + L

which will result in a new form and now problems.

Johan

> > >>>>> 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?
> > >>>>> 
> > >> The code that I disabled was caching preprocessed forms, so I don't
> > >> see how this can be handled by Instant.
> > >> 
> > >> Garth
> > >>

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 23:10:59 +0100

> None) which would trigger the Instant disk cache.

This is the problem - how do we know that it's changed?

The original issue is not related to disk vs memory cache, or Instant.
It is how to avoid calling ufl.preprocess unnecessarily when the form
repr() is unchanged but the mesh has been changed.

Garth

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 15:14:45 -0700

On Monday April 25 2011 15:04:43 Anders Logg wrote:
> > >>>>>>>>>>>>> Commenting out the cache is really not a fix. The problem
> > >>>>>>>>>>>>> is within dolfin. Isn't there another way to deal with
> > >>>>>>>>>>>>> this?
> > >>>>>>>>>>>> 
> > >>>>>>>>>>>> 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.
> > >>>>>>>>>>> 
> > >>>>>>>>>>> creates a canonical version of the Form so two Forms that
> > >>>>>>>>>>> are the same, but constructed at different times are beeing
> > >>>>>>>>>>> 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.
> > >>>>>>>>>> 
> > >>>>>>>>>> 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.
> > >>>>>>> 
> > >>>>>>> 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:
> > >>>>>>> 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?
> > >>>>>>> 
> > >>>>>>> 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.
> > >>>>>> 
> > >>>>> 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.
> > >>>>> 
> > >>>>> does not need to any significant speedup. The JIT benchmark runs
> > >>>>> just as fast if I call signature from within __hash__.
> > >>>>> 
> > >>>>> 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).
> > >>>>> 
> > >>>>> 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 issue that I ran into was not related to signatures. It was
> > >> related to the non-UFL data that is attached to arguments.
> > > 

Sounds complicated...

Now the preprocessed form is stored in the original form. This will never 
change. Whenever a form does not go out of scope the preprocessed form will 
live. 

Also Martin made it impossible to change a form without returning a new 
instance. This prevents any changing of the original form while keeping a 
preprocesses form attached to it.

If a form has a preprocessed form that will be used for code generation. The 
preprocessed form will be used in instants memory cache. The preprocessed form 
has nothing to do the any DOLFIN objects that comes with the original form, 
such as mesh, expressions and such.

Anything I have missed?

Johan

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 00:16:01 +0200

> This is the problem - how do we know that it's changed?

Because we change it. When we modify a Form, we need to invalidate the
hash, by making sure that we modify the Form by calling member
functions that invalidate the hash (setting it to None).

> repr() is unchanged but the mesh has been changed.

Exactly, and that's why I suggest introducing the hash as something to
be checked instead of id() which turned out not to be robust.

And furthermore, the call to id/hash should be handled by Instant, not
FFC. Since Instant handles JIT compiling and caching, FFC should only
need to call Instant, not handle extra caching.

--
Anders

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 23:16:06 +0100

> which will result in a new form and now problems.

Have you tested it? Your approach is more elegant than my hack, so it
will probably work. I assigned the preprocessed form to the original, i.e.

  a_tmp = preprocess(a)
  a_tmp += a

which broke, I think because of the index renumbering performed by
preprocess.

Garth

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 00:19:20 +0200

> > None) which would trigger the Instant disk cache.

I think it sounds very easy. Everything we need is there: Instant
already has memory and disk cache. We just need to provide the proper
input.

> such as mesh, expressions and such.

What about the __hash__ function in jitobject.py? It still calls
id(). Isn't that a problem?

--
Anders

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 23:19:36 +0100

> functions that invalidate the hash (setting it to None).

It's the ufl form that is changed, not the DOLFIN form.

Garth

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 15:22:15 -0700

On Monday April 25 2011 15:16:06 Garth N. Wells wrote:
> >> Oups! I guess my "fix" wont get this either... Maybe we need to destroy
> >> the form_data object whenever += ... are called?
> > 
> > I do not think this will be a problem, as += here returns a new object. I
> > am actually not sure how this works, but I think because __iadd__ is
> > _not_
> > 
> Have you tested it? 

Yes.

> Your approach is more elegant than my hack, so it

Yes, we shall not expose the preprocessed form to the user.

Johan

> >>>>>>>> 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
> >>>>>> >From some simple tests I did now, it looks like the __hash__
> >>>>>> >
> >>>>>>> function
> >>>>>> 
> >>>>>> and on-disk, by providing two functions __hash__ (fast, for
> >>>>>> in-memory cache) and signature (slow, for on-disk cache).
> >>>>>>

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 23:23:42 +0100

> id(). Isn't that a problem?

Yes, but we're trying to tackle one issue at a time. First avoiding
necessary calls to preprocess. Note that there will be common usage
cases in which a call to preprocess in needed, but for which the
compiled from can be extract from the Instant cache.

Garth

---

From: "Garth N. Wells" <[email-masked]>
Date: Mon, 25 Apr 2011 23:25:31 +0100

> preprocesses form attached to it.

Yes, and Martin wasn't too happy with some UFL changes in his absence
that broke this design. It is a lot cleaner not modifying objects
post-construction.

Garth

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 15:28:30 -0700

On Monday April 25 2011 15:19:20 Anders Logg wrote:
> > > > >>>>>>>>>>>>> Commenting out the cache is really not a fix. The
> > > > >>>>>>>>>>>>> problem is within dolfin. Isn't there another way to
> > > > >>>>>>>>>>>>> deal with this?
> > > > >>>>>>>>>>>> 
> > > > >>>>>>>>>>>>> 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.
> > > > >>>>>>> 
> > > > >>>>>>> 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?
> > > > >>>>>>> 
> > > > >>>>>>>> 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):
> > > > >>>>>> 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.
> > > > >>>>> 
> > > > >>>>> does not need to any significant speedup. The JIT benchmark
> > > > >>>>> runs just as fast if I call signature from within __hash__.
> > > > >>>>> 
> > > > >>>>> 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.
> > > > >>> 
> > > > >>> The point would be that one could check that "hash" of the form
> > > > >>> (some unique string) instead of computing the signature which
> > > > >>> involves preprocessing the form.
> > > > >> 
> > > > >> How would the hash be computed? To check if the mesh has changed,
> > > > >> my limited understanding is that the entire object would have to
> > > > >> be serialised, and then a hash computed. How expensive is that?
> > > > >> 
> > > > But how does Instant know if a form is new? I also don't see why
> > > > Instant should need to know if the mesh associated with a form has
> > > > changed, but is for the rest the same. Wouldn't Instant need to be
> > > > DOLFIN-aware?
> > > 

The set and get hash introduce another level of complexity.

> > change. Whenever a form does not go out of scope the preprocessed form
> > will live.
> > 
> > If a form has a preprocessed form that will be used for code generation.
> > The preprocessed form will be used in instants memory cache. The
> > preprocessed form has nothing to do the any DOLFIN objects that comes
> > with the original form, such as mesh, expressions and such.
> > 
> > Anything I have missed?
> 

No. The __hash__ is only used to retreive the memory cached compiled module in 
instant. This so we can retreive the compiled form. The form_data including 
all DOLFIN dependent data are stored in form_data which is just retrieved from 
the preprocessed form.

A user can change this, but it seems that form_data is correctly updated with 
the new mesh.

Btw: We could probably store the compiled form also in form_data. We could 
then avoid going through instant memory cache :)

Johan

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 00:34:55 +0200

> all DOLFIN dependent data are stored in form_data which is just retrieved from

Why is this not problematic if a new form is created which happens to
get the same id as an old destroyed form? I thought that was the
original issue.

--
Anders (offline until tomorrow)

---

From: Johan Hake <[email-masked]>
Date: Mon, 25 Apr 2011 15:41:16 -0700

On Monday April 25 2011 15:34:55 Anders Logg wrote:
> > > > > > >>>>>>>>>>>>> problem is within dolfin. Isn't there another way
> > > > > > >>>>>>>>>>>>> to deal with this?
> > > > > > >>>>>>>>>>>> 
> > > > > > >>>>>>>>>>>> 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.
> > > > > > >>>>>>>>>>> 
> > > > > > >>>>>>>>>>> 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:
> > > > > > >>>>>>> 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
> > > > > > >>>>>>> instances owned by the Form. I guess this is similare to
> > > > > > >>>>>>> the difficulties of preserving constness in C++, but I
> > > > > > >>>>>> What if we have the FFC jit compiler return the
> > > > > > >>>>>> preprocessed form, and inside dolfin.Form simply do
> > > > > > >>>>>> 
> > > > > > >>>>>>     class Form(cpp.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.
> > > > > > >>>>> 
> > > > > > >>>>> Furthermore, the __hash__ function must also be broken
> > > > > > >>>>> since it relies on calling id on the form.
> > > > > > >>>>> 
> > > > > > >>>>> (fast, for in-memory cache) and signature (slow, for
> > > > > > >>>>> on-disk cache).
> > > > > > >>>>> 
> > > > > > >>>>> 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.
> > > > > > >>>>> 
> > > > > > >>>> The code that I disabled was caching preprocessed forms, so
> > > > > > >>>> I don't see how this can be handled by Instant.
> > > > > > >>> 
> > > > > > >>> The point would be that one could check that "hash" of the
> > > > > > >>> form (some unique string) instead of computing the signature
> > > > > > >>> which involves preprocessing the form.
> > > > > > >> 
> > > > > > >> How would the hash be computed? To check if the mesh has
> > > > > > >> changed, my limited understanding is that the entire object
> > > > > > >> would have to be serialised, and then a hash computed. How
> > > > > > >> expensive is that?
> > > > > > >> 
> > > > > > >> The issue that I ran into was not related to signatures. It
> > > > > > >> was related to the non-UFL data that is attached to
> > > > > > >> arguments.
> > > > > > > 
> > > > > > > The hash would be unique to each form. It could just be a
> > > > > > > counter value and the counter would be increased inside
> > > > > > > Instant for each object it gets as input.
> > > > > > 
> > > > > > Instant should need to know if the mesh associated with a form
> > > > > > has changed, but is for the rest the same. Wouldn't Instant need
> > > > > > to be DOLFIN-aware?
> > > > > 
> > > > > We would need to make sure from the DOLFIN side that when we change
> > > > > a Form, we also change the hash value (for example by setting it
> > > > > to None) which would trigger the Instant disk cache.
> > > > 
> > > > Sounds complicated...
> > > 
> > > > Now the preprocessed form is stored in the original form. This will
> > > > never change. Whenever a form does not go out of scope the
> > > > preprocessed form will live.
> > > > 
> > > > Also Martin made it impossible to change a form without returning a
> > > > new instance. This prevents any changing of the original form while
> > > > keeping a preprocesses form attached to it.
> > > > 
> > > > If a form has a preprocessed form that will be used for code
> > > > generation. The preprocessed form will be used in instants memory
> > > > cache. The preprocessed form has nothing to do the any DOLFIN
> > > > objects that comes with the original form, such as mesh, expressions
> > > > and such.
> > > > 
> > > > Anything I have missed?
> > > 
> > No. The __hash__ is only used to retreive the memory cached compiled
> > module in instant. This so we can retreive the compiled form. The
> > form_data including all DOLFIN dependent data are stored in form_data
> > which is just retrieved from the preprocessed form.
> 

Good point. The hash is only used to retreive the compiled module from instant 
memory cache, right? Then I think we can safely base the hash on the 
signature.
 
Johan

> > A user can change this, but it seems that form_data is correctly updated
> > with the new mesh.
> > 
> > Btw: We could probably store the compiled form also in form_data. We
> > could then avoid going through instant memory cache :)
> > 
> > Johan

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 09:01:04 +0200

> that

(Not sure if I got the quote author correct here.)

The ufl Form does not have a _count...

The _count of e.g. Coefficient objects is a critical property, since a(u, v)
!= a(v, u).

My hunch is that this is yet another PyDOLFIN/UFL integration issue?

As a general comment, the instant cache is for the generated modules,
not runtime state of the module calling upon instant. I think the caches
in instant should only represent the ufl->ffc->ufc "mapping", not including
dolfin and ffc state.

I can't comment further on this thread without understanding what the
problem is.

Martin

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 08:20:57 +0100

Martin: Any problem if we apply this patch to UFL?

Garth

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 10:01:55 +0200

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!

I don't know which memory cache you're referring to here.
Instant has a memory cache to avoid unnecessary disk access.

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?

Martin

On 25 April 2011 23:50, Johan Hake <[email-masked]> wrote:

> is
> form
> could
> the
> then
> be
> the
> a
> first
> just
> in-memory
> in-memory
> and

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 10:03:42 +0200

See other mail. I don't see that it solves anything, it doesn't seem related
to anything I've read about in this thread, and it has a potential backside
in hindering the garbage collector in Python. I may be wrong, but nobody has
answered my other questions about this thread yet.

Martin

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 09:56:18 +0100

On 26/04/11 09:03, Martin Sandve Alnæs wrote:
> See other mail. I don't see that it solves anything, it doesn't seem
> related to anything I've read about in this thread, and it has a
> potential backside in hindering the garbage collector in Python. I may
> be wrong, but nobody has answered my other questions about this thread yet.
>

As a precursor, the primary problem has nothing to do with Instant disk
cache, etc. The Instant discussion is just confusing the original point.

In summary, is it helpful if DOLFIN can avoid calling ufl.preprocess
every time a dolfin.Form object is created. DOLFIN relies on
preprocessing to extract the form Arguments, from which the mesh is
extracted (via form_data().original_arguments, and since DOLFIN uses
'Arguments' that are subclasses of UFL and DOLFIN objects).

The solution that Johan has implemented is to have FFC attach the
form_data to a form. If a form has form_data attached, then we know that
it has already been preprocessed. Martin won't like this because it's
changing the form object.

It may be enough if UFL would provide a function to return a list of
form Arguments, if this is fast. Something like

  def extract_original_arguments(form):

      # Replace arguments and coefficients with new renumbered objects
      arguments, coefficients = extract_arguments_and_coefficients(form)
      replace_map, arguments, coefficients \
            = build_argument_replace_map(arguments, coefficients)
      form = replace(form, replace_map)

      # Build mapping to original arguments and coefficients, which is
      # useful if the original arguments have data attached to them
      inv_replace_map = {}
      for v, w in replace_map.iteritems():
          inv_replace_map[w] = v
      original_arguments = [inv_replace_map[v] for v in arguments]

      return original_arguments 	

Garth

> Martin
> 
> On 26 April 2011 09:20, Garth N. Wells <[email-masked]
> <mailto:[email-masked]>> wrote:
> 
>     problem is
>     a form
>     is then
>     will be
>     form? I
>     object.
>     prohibits a
>     runs just
>     Instant) and
>     <mailto:[email-masked]>
>     Post to     : [email-masked] <mailto:[email-masked]>

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 10:09:35 +0100

>       return original_arguments 	

As addition, I think that we're letting DOLFIN specific issues creep
into FFC and UFL. It would be simple if FFC simply did

    if form.form_data() is not None:
        preprocessed_form = form
    else:
        preprocessed_form = preprocess(form, common_cell=common_cell)

and DOLFIN is made responsible for preprocessing a form (or not
preprocessing) before sending it to the FFC JIT compiler, particularly
since deciding to preprocess or not can depend on what DOLFIN-specific
data (e.g. meshes) has been attached to the form.

Garth

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 13:22:28 +0200

> changing the form object.

This sounds much like my original design. Trying to recall from my possibly
rusty memory, I believe that calling myform.form_data() would
construct form data only the first time and the preprocessed form could
be retrieved from the returned form data. The form data was attached
as myform._form_data. Thus you could always say
preprocessed_form = myform.form_data().form
and preprocessing would only happen once. This was redesigned
after I left to have a separate preprocess function.

>      return original_arguments

I don't understand why this is needed. We:
- must preprocess each form once
- don't want to preprocess the same form twice
- can obtain the original arguments after preprocessing
This was supported a long time ago, so unless someone has
removed functionality while I've been gone, what is the problem?

I have a feeling that the source of many problems is the attempt
to reuse forms and change mesh, functions, or elements.
This is contrary to the design of UFL where expressions are immutable.

Martin

> time.
> by
> two
> unique

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 13:25:59 +0200

>        preprocessed_form = preprocess(form, common_cell=common_cell)
Is this ufl.preprocess or ffc.preprocess?

> data (e.g. meshes) has been attached to the form.

Here I'm lost. Why is preprocessing dependent on dolfin anything?
What kind of preprocessing are we talking about here?

Martin

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 12:35:40 +0100

On 26/04/11 12:25, Martin Sandve Alnæs wrote:
> On 26 April 2011 11:09, Garth N. Wells <[email-masked]

ufl.preprocess. There is no ffc.preprocess as far as I know.

> What kind of preprocessing are we talking about here?

Because DOLFIN subclasses ufl.Coefficient, ufl.Argument, e.g. from the
DOLFIN code

  class Argument(ufl.Argument):
      def __init__(self, V, index=None):
          if not isinstance(V, FunctionSpaceBase):
              raise TypeError, "Illegal argument for creation of
	                    Argument, not a FunctionSpace: " + str(V)
          ufl.Argument.__init__(self, V.ufl_element(), index)
          self._V = V

      def function_space(self):
          "Return the FunctionSpace"
          return self._V

DOLFIN relies on UFL to extract the Arguments from a form, and then
DOLFIN calls the above member function 'function_space' to extract the
DOLFIN function space.

Garth

>     <mailto:[email-masked]>
>     >> <mailto:[email-masked] <mailto:[email-masked]>>> wrote:
>     >>
>     >>     > I do not see why we introduced the memory cache when this
>     solution
>     >>     was laying
>     preprocessing by
>     >>     >>>>>>>>>> robust way to make sure that the form is the same
>     as one
>     >>     in the
>     >>     >>>>>>>>>> the form, which then defeats the purpose of a
>     cache. It
>     >>     may be
>     >>     >>>>>>> stored in memory cache. If we are able to strip such
>     a form
>     >>     for all
>     >>     >>>>>>> could then use that repr to generate a form which is
>     stored
>     >>     in the
>     >>     >>>>>>> user to change attributes on instances owned by the
>     Form. I
>     >>     guess
>     preprocessed
>     >>     >>>>> If Martin does not like that, we should be able to
>     handle it
>     >>     on the
>     >>     >>>>> DOLFIN side.
>     >>     >>>>>
>     >>     <mailto:[email-masked] <mailto:[email-masked]>>
>     <mailto:[email-masked]> <mailto:[email-masked]
>     <mailto:[email-masked]>>

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 13:41:22 +0200

On Tue, Apr 26, 2011 at 01:22:28PM +0200, Martin Sandve Alnæs wrote:
>     On 26/04/11 09:03, Martin Sandve Aln s wrote:

It was removed but because we (or maybe only I?) thought the
self-caching of form data was too much magic and that it was more
straightforward to have

  preprocessed_form = preprocess(form)

This requires that any caching is handled from the outside. But it
seems now that caching is problematic (since we cannot rely on id() or
any other simple way to identify the form). Perhaps the best solution
is just to put it back? Then form compilers can safely call
preprocess() without needing to check whether the form data exists.

I still think that a call to preprocess should be necessary since
it's a potentially expensive operation. (The preprocess function could
call form._data() which can return None if the data is not there and
otherwise generate the data and attach it.)

--
Anders

>     >     Post to     : [email-masked] <mailto:[email-masked]
>     >

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 12:43:06 +0100

On 26/04/11 12:22, Martin Sandve Alnæs wrote:
> On 26 April 2011 10:56, Garth N. Wells <[email-masked]
> and preprocessing would only happen once. 

I think that the above would solve the issue. At the moment ufl.Form has
the member function:

   def form_data(self):

If it did

   def form_data(self):
	if self._form_data is None:
            # compute form_data 	
        return self._form_data

it should make things straightforward. But doesn't this violate
immutability of the form, or is it ok since the mathematical form itself
is not being modified?

Garth

> This was redesigned
>     This
>     >     >>>>>>> stored in memory cache. If we are able to strip such a
>     form
>     >     for all
>     have two

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 13:46:26 +0200

> immutability of the form, or is it ok since the mathematical form itself

I think it is ok for the same reason that dolfin::mesh.init() is
const: the mesh does not change and the edges are already there (only
not yet extracted). Same with the form: the metadata is there but not
yet extracted.

--
Anders

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 13:46:59 +0200

Caching is ok precicely _because_ the form is immutable.

Martin

On 26 April 2011 13:43, Garth N. Wells <[email-masked]> wrote:

> >     >     >>>>>>>>>>>> preprocessing, but it was highly dubious and I
> doubt
> >     >     that it
> >     >     >>>>>>>>>>>> was effective.
> >     >     >>>>>>>>>>>
> >     >     >>>>>>>>>>> creates a canonical version of the Form so two
> Forms
> >     >     that are
> >     >     >>>>>>>>>> the form, which then defeats the purpose of a cache.
> It
> >     >     may be
> >     >     >>>>>>> DOLFIN specific things we would also prevent huge
> memory
> >     >     leaks with
> >     >     >>>>>>> mesh beeing kept.
> >     >     >>>>>>>
> for
> >     >     internal
> >     >     >>>>> 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.
> >     >     >>>>>

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 13:55:22 +0200

So will you add it back? I can do it since it's my fault removing it,
but I assume you want to handle it yourself. ;-)

--
Anders

On Tue, Apr 26, 2011 at 01:46:59PM +0200, Martin Sandve Alnæs wrote:
>     On 26/04/11 12:22, Martin Sandve Aln s wrote:
>     >     >     <mailto:[email-masked] <mailto:[email-masked]
>     >>

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 13:57:40 +0200

Feel free to do it :)

Martin

On 26 April 2011 13:55, Anders Logg <[email-masked]> wrote:

> >     >     > See other mail. I don't see that it solves anything, it
> doesn't
> >     seem
> >     >     > potential backside in hindering the garbage collector in
> Python. I
> >     may
> >     >     > be wrong, but nobody has answered my other questions about
> this
> >     >     thread yet.
> >     >     >
> >     >
> >     >     As a precursor, the primary problem has nothing to do with
> Instant
> >     disk
> >     >     cache, etc. The Instant discussion is just confusing the
> original
> >     point.
> >     >
> >     >     form_data to a form. If a form has form_data attached, then we
> know
> >     that
> >     >          arguments, coefficients =
> extract_arguments_and_coefficients
> >     (form)
> >     >     >     > Anyhow. Here is a patch for ufl to avoid circular
> dependency
> >     >     between a
> >     >     >     >>>>> On Mon, Apr 25, 2011 at 07:40:21PM -0000, Garth
> Wells
> >     wrote:
> >     >     >     >>>>>>>>>>>>> within dolfin. Isn't there another way to
> deal
> >     >     with this?
> >     >     >     >>>>>>>>>>>>
> >     >     >     >>>>>>>>>>>> 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
> >     >     >     >>>>>>>>>>> I think the preprocessing stage actually do
> take
> >     >     some time.
> >     >     >     >>>>>>>>>>> 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
> >     >     >     >>>>>>>>>> keeping the object in scope. If the object
> changes,
> >     >     the only
> >     >     >     >>>>>>>>>> cache is to compare all the data. This
> requires
> >     >     preprocessing
> >     >     >     >>>>>>>>>> possible to add a lightweight preprocess to
> UFL,
> >     >     but I don't
> >     >     >     >>>>>>> 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
> >     >     >     >>>>>>> include a second repr function, which did not
> include
> >     >     the count?
> >     >     >     >>>>>>> This would then be used when the signature is
> checked
> >     >     for. We
> >     >     >     >>>>>>> objects. This should work as the _count attribute
> has
> >     >     nothing to
> >     >     >     >>>>>>>> An easy fix is to attach the preprocessed form
> to a
> >     Form
> >     >     >     object.
> >     >     >     >>>>>>> user to write to these but it might not be
> possible to
> >     >     >     prohibit a
> >     >     >     >>>>>>> this is similare to the difficulties of
> preserving
> >     >     constness in
> >     >     >     >>>>> is used to located objects (generated code/modules)
> in
> >     >     the Instant
> >     >     >     >>>>> >From some simple tests I did now, it looks like
> the
> >     >     __hash__
> >     >     >     function
> >     >     >     >>>>>>
> >     >     >     >>>>> Ideally, we should get Instant to handle the
> caching,
> >     both
> >     >     >     in-memory
> >     >     >     >>>>> and on-disk, by providing two functions __hash__
> (fast,
> >     for
> >     >     >     in-memory
> >     >     >     >>>>> 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
> handle
> >     >     >     >>>>> 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
> >     >     >     >>>>> 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.
> >     >     >     >>>>>

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 14:00:50 +0200

It feels good that you trust me enough to handle it. ;-)

Will add it sometime this afternoon and then we can revisit the JIT
compiler caching.

--
Anders

On Tue, Apr 26, 2011 at 01:57:40PM +0200, Martin Sandve Alnæs wrote:
>     On Tue, Apr 26, 2011 at 01:46:59PM +0200, Martin Sandve Aln s wrote:

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 14:51:21 +0200

> Will add it sometime this afternoon and then we can revisit the JIT

I'm getting confused here... Looking at preprocess.py in UFL, I see this:

def preprocess(form, object_names={}, common_cell=None):

    ...

    # Check that form is not already preprocessed
    if form.form_data() is not None:
        debug("Form is already preprocessed. Not updating form data.")
        return form

    ...

    # Attach form data to form
    form._form_data = form_data

    # Attach preprocessed form to form data
    form_data._form = form

And when I look at the blamelist (bzr annotate), it looks like I added
those lines, so I must have come to my senses and added it back at
some point (way back). So in conclusion, calling preprocess() should
not taking any time.

What am I missing?

--
Anders

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 15:45:22 +0100

> I'm getting confused here... Looking at preprocess.py in UFL, I see this:

It is confusing. Does the function 'preprocess' do anything that the old
FormData class didn't? It would be easier to follow if Form just had a
member function form_data() that computes and stores data (like it used
to), or if Form had a 'preprocess' function. Having the function
preprocess return a new form is really confusing.

Garth

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 16:55:44 +0200

> preprocess return a new form is really confusing.

I don't find that particularly confusing. It's the same as

  refined_mesh = refine(mesh)

--
Anders

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 07:59:26 -0700

On Tuesday April 26 2011 00:01:04 Martin Sandve Alnæs wrote:
> The _count of e.g. Coefficient objects is a critical property, since a(u,
> v) != a(v, u).
> 

Do not bother. It was a try for me to get into the original problem.

Yes.
 
> in instant should only represent the ufl->ffc->ufc "mapping", not including

Agree.

Johan

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 15:59:52 +0100

>   refined_mesh = refine(mesh)

Which is the whole problem. By creating a new object, FormData is thrown
away. The preprocessing should just compute some more data, just like we
*don't* do

  initialised_mesh = mesh.init(0)

What was wrong with Martin's original design that necessitated the change?

Garth

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 17:07:47 +0200

> What was wrong with Martin's original design that necessitated the change?

As I explained, I thought it was better to have an explicit call to
preprocess since that makes it clear that one makes a call to a
function which may take some time to execute (instead of just calling
a member function which seems to just return some data).

But as I say above: I added the caching back at some point (maybe even
the day after I removed it 2 years ago) so we don't need to discuss
why I removed it (as I realized myself I shouldn't have removed it and
added it back a long time ago).

What has me confused now is that the caching seems to be in place but
we still need the extra caching in FFC/DOLFIN and I don't see why.

--
Anders

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:08:56 -0700

> 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.

> 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.

> 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

> > > > > >>>>>>>> is only preprocessed if it hasn't already been
> > > > > >>>>>>> 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
> > > > > >>>>>>> consider putting another cache in place which is more
> > > > > >>>>>> cache is to compare all the data. This requires
> > > > > >>>>>> 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.
> > > > > >>> 
> > > > > >>> 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.
> > > > > >>> 
> > > > > >>> 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
> > > > > >>> nothing to do with what code gets generated, but it is
> > > > > >>> essential for
> > 
> > internal
> > 
> > > > > >>> UFL algorithms, right?
> > > > > >>> 
> > > > > >>>> immutable once they've been compiled. Is it possible to make
> > > > > >>>> a Python object immutable?
> > > > > >>> 
> > > > > >>> prohibits
> > > > > >>> user to write to these but it might not be possible to prohibit
> > > > > > is used to located objects (generated code/modules) in the
> > > > > > Instant in-memory cache, while the second is used for the
> > > > > > on-disk cache.
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > FFC attaches a hash to incoming forms so that Instant may
> > > > > > recognize them later.
> > > > > 
> > see
> > > > > > 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.
> > > > > >

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 16:16:29 +0100

> we still need the extra caching in FFC/DOLFIN and I don't see why.

Because preprocess returns a new form, e.g. define a form

  a = u*v*dx
  jit(a)

Inside jit,

   a.form_data() is None:
       b = preprocess(a) # b now has data attached, but a doesn't
   else:
       b = a

Now 'b' has been preprocessed, and has form data attached, but 'a'
doesn't. Calling 'jit(a)' again, the code will never enter the 'else'
part of the clause because 'a' never gets any form data. Johan has added
some code FFC that attaches the form data of 'b' to 'a', but it is a bit
clumsy. Better would be

    a.preprocess()

or

    a.form_data()

Garth

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:18:01 -0700

Why can't we just go with the original design of Martin?

In jit_compiler() we just call:

form_data = form.form_data()

This will generate the form data, including the preprocessed form attached to 
it, if the form_data is not already generated. 

Then we get the preprocessed form by:

  form_data.form

We need to add a call to preprocess within Form.form_data(), (if it is not 
already generated), and we need to remove the reference stored by the 
preprocessed form to the form_data to avoid circular dependency. We also need 
to make preprocess return the form_data instead of the preprocessed form.

Johan

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:21:07 -0700

On Tuesday April 26 2011 08:07:47 Anders Logg wrote:
> > >> It is confusing. Does the function 'preprocess' do anything that the
> > >> old FormData class didn't? It would be easier to follow if Form just
> > >> had a member function form_data() that computes and stores data (like
> > >> it used to), or if Form had a 'preprocess' function. Having the
> > >> function preprocess return a new form is really confusing.
> > > 

What extra caching? The caching of preprocessed forms are removed from FFC 
with no penalty in speed, because we no fallback on something similare to 
Martins original design. But as stated in my other email we need to do some 
more fixes to get it going without potential memory leaks.

Johan

> > >>>         debug("Form is already preprocessed. Not updating form
> > >>>         data.") return form
> > >>>     
> > >>>     ...
> > >>>     
> > >>> And when I look at the blamelist (bzr annotate), it looks like I
> > >>> added those lines, so I must have come to my senses and added it
> > >>> back at some point (way back). So in conclusion, calling
> > >>> preprocess() should not taking any time.
> > >>> 
> > >>> What am I missing?
>

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:31:04 -0700

On Tuesday April 26 2011 08:16:29 Garth N. Wells wrote:
> clumsy. 

No, it was already attached. I just made ffc use it.

> Better would be

As already mentioned in a previous email, I suggest we only call form_data(). 
This will return the form_data. The preprocessed form is attached to the 
form_data and this is what is passed to the code generator. I am pretty sure 
this is what was there from the beginning.

It is confusing to call:

  form = preprocess(form)

as the preprocessed form was never ment to be doing anything but being passed 
to the code generator, AFAIK.

Johan

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 17:32:24 +0200

Is no one listening???

Look at preprocess.py in UFL. It caches the preprocessed form and the
preprocessed form holds the form data. This is the same as Martin's
original design as far as I can tell.

The difference is that a call to preprocess is necessary but that just
returns the cached form.

--
Anders

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 16:33:11 +0100

> No, it was already attached. I just made ffc use it.

Didn't you add the line

    form._form_data = preprocessed_form.form_data()

?

Garth

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:39:30 -0700

On Tuesday April 26 2011 08:33:11 Garth N. Wells wrote:
> >>>>>>>> Will add it sometime this afternoon and then we can revisit the
> >>>>>>>> JIT compiler caching.
> >>>>>>> 
> >>>>>>> I'm getting confused here... Looking at preprocess.py in UFL, I see
> > 
> > this:
> >>>>>> had a member function form_data() that computes and stores data
> >>>>>> (like it used to), or if Form had a 'preprocess' function. Having
> >>>>>> the function preprocess return a new form is really confusing.
> >>>>> 
> >>>> Which is the whole problem. By creating a new object, FormData is
> >>>> thrown away. The preprocessing should just compute some more data,
> >>>> just like we *don't* do
> >>>> 

No, I added:

  preprocessed_form = form.form_data()._form

I think the thing here is that form_data has always had a preprocessed form. 
Someone (lets not point fingers!) thought that was too much magic and added an 
explicit need to call:

  form = preprocess(form)

in jit_compiler(). This made the design more complicated and also introduced a 
cirucular dependency, as the return preprocessed form need to know of its 
form_data, but the form_data already had a reference to the preprocessed form. 
The latter is what I used in the one line I altered.

Johan
 
> ?
> 
> Garth
> 
> >> Better would be
> >> 
> >>     a.preprocess()
> >> 
> >> or
> >> 
> >>     a.form_data()
> > 
> > As already mentioned in a previous email, I suggest we only call
> > form_data(). This will return the form_data. The preprocessed form is
> > attached to the form_data and this is what is passed to the code
> > generator. I am pretty sure this is what was there from the beginning.
> > 
> > It is confusing to call:
> >   form = preprocess(form)
> > 
> > as the preprocessed form was never ment to be doing anything but being
> > passed to the code generator, AFAIK.
> > 
> > Johan
> > 
> >> Garth
> >> 
> >>> --
> >>> Anders
> >>> 
> >>>> Garth
> >>>> 
> >>>>>> Garth
> >>>>>>

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 16:39:45 +0100

> original design as far as I can tell.

Put some print statements and you'll see that the cache doesn't work.
Print the id of the object form either side of

    form = expand_derivatives(form)

in preprocess.py. This returns a new object, so the form that is passed
into preprocess.py as a function argument does not get data attached.
This leads to the issue that I sketched

Garth

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 17:42:32 +0200

> The latter is what I used in the one line I altered.

No, it made the design cleaner since it makes clear something needs to
happen to get the metadata: a call to preprocess.

Where did you add this line?

--
Anders

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:43:20 -0700

> Is no one listening???

I am.
 
> The difference is that a call to preprocess is necessary but that just

I think preprocess should return the form_data and _not_ the preprocessed 
form. The preprocessed form should be (and are) attached to form_data. The 
form_data method should then be update to:

 def form_data(self):
     if self._form_data is None:
         self._form_data = preprocessed(self)
     return self._form_data

then in jit_compile() we just do:

  form_data = form.form_data()
  preprocessed_form = form_data._form

removing all logic from jit_compile.

Johan

> > This will generate the form data, including the preprocessed form
> > attached to it, if the form_data is not already generated.
> > 
> > We need to add a call to preprocess within Form.form_data(), (if it is
> > not already generated), and we need to remove the reference stored by
> > the preprocessed form to the form_data to avoid circular dependency. We
> > also need to make preprocess return the form_data instead of the
> > preprocessed form.
> > 
> > Johan
> >

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:44:24 -0700

On Tuesday April 26 2011 08:42:32 Anders Logg wrote:
> > > >>>>>>>> Will add it sometime this afternoon and then we can revisit
> > > >>>>>>>> the JIT compiler caching.
> > > >>>>>>> 
> > > >>>>>> It is confusing. Does the function 'preprocess' do anything that
> > > >>>>>> the old FormData class didn't? It would be easier to follow if
> > > >>>>>> Form just had a member function form_data() that computes and
> > > >>>>>> stores data (like it used to), or if Form had a 'preprocess'
> > > >>>>>> function. Having the function preprocess return a new form is
> > > >>>>>> really confusing.
> > > >>>>> 
> > > >>> function which may take some time to execute (instead of just
> > > >>> calling a member function which seems to just return some data).
> > > >>> 
> > > >>> But as I say above: I added the caching back at some point (maybe
> > > >>> even the day after I removed it 2 years ago) so we don't need to
> > > >>> discuss why I removed it (as I realized myself I shouldn't have
> > > >>> removed it and added it back a long time ago).
> > > >>> 
> > > >>> What has me confused now is that the caching seems to be in place
> > > >>> but we still need the extra caching in FFC/DOLFIN and I don't see
> > > >>> why.
> > > >> 
> > > >> doesn't. Calling 'jit(a)' again, the code will never enter the
> > > >> 'else' part of the clause because 'a' never gets any form data.
> > > >> Johan has added some code FFC that attaches the form data of 'b' to
> > > >> 'a', but it is a bit clumsy.
> > > > 
> > I think the thing here is that form_data has always had a preprocessed
> > form. Someone (lets not point fingers!) thought that was too much magic
> > and added an
> > 
> > explicit need to call:
> >   form = preprocess(form)
> > 
> > in jit_compiler(). This made the design more complicated and also
> > introduced a cirucular dependency, as the return preprocessed form need
> > to know of its form_data, but the form_data already had a reference to
> > the preprocessed form. The latter is what I used in the one line I
> > altered.
> 

I change

  preprocessed_form = form

to:

Johan

> > > > as the preprocessed form was never ment to be doing anything but
> > > > being passed to the code generator, AFAIK.
> > > > 
> > > > Johan
> > > > 
> > > >> Garth
> > > >> 
> > > >>>> Garth
> > > >>>> 
> > > >>>>>> Garth
> > > >>>>>> 
> > > >>>>>>> And when I look at the blamelist (bzr annotate), it looks like
> > > >>>>>>> I added those lines, so I must have come to my senses and
> > > >>>>>>> added it back at some point (way back). So in conclusion,
> > > >>>>>>> calling preprocess() should not taking any time.
> > > >>>>>>> 
> > > >>>>>>> What am I missing?
> > > >>

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 17:47:55 +0200

> reference to the preprocessed form => Ciruclar dependency.

I.e. a.b = b; b.a = a.

> happens the preprocessed form will be deleted (I guess).

A guess is not enough :)

I don't feel confident about the __del__ patch.
As you say, removing the circular references sounds like a better solution.

Martin

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 16:48:33 +0100

>> happen to get the metadata: a call to preprocess.

How about something like

  a.compute_form_data()

to compute the data, and

  data = a.form_data()

to get the FormData. This is like Martin's orginal design, except
form_data() returns None if the data hasn't been computed.

Garth

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 17:50:31 +0200

> As you say, removing the circular references sounds like a better solution.

A possible tool for breaking the cycle:
http://docs.python.org/library/weakref.html

Martin

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 08:55:54 -0700

> >> happens the preprocessed form will be deleted (I guess).

We now have:

  a.b = b
  b.a = a
  c.a = a

when c is deleted it deletes b from a. So I guess it would work.

> > As you say, removing the circular references sounds like a better

Yes, that would be much better!

> http://docs.python.org/library/weakref.html

Yes, that would help.

Johan

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 18:01:35 +0200

>   preprocessed_form = form.form_data()._form

Yes, but where?

I've fixed the bug now in preprocess.py (attaching to both forms). Does
that help?

--
Anders

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 09:01:37 -0700

> >> happen to get the metadata: a call to preprocess.

Why is:

so bad? 

> form_data() returns None if the data hasn't been computed.

I think this adds more to the form than is nessesary. 

Johan

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 18:03:55 +0200

>   preprocessed_form = form_data._form

Since it makes it look like form_data() just returns existing data
when it actually leads to an expensive computation.

--
Anders

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 18:07:02 +0200

If you want to play around with weakref to understand it, try this snippet:

import weakref

class A:
def __init__(s, n):
s.n = n
def __del__(s):
print "d", s.n

a = A("orig")
b = A("prep")
c = A("formdata")
a.fd = c
b.fd = c
c.a = weakref.ref(a)
c.b = weakref.ref(b)
print "C"
del c
print "A"
del a
print "B"
del b
print "DONE"

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 09:07:43 -0700

On Tuesday April 26 2011 09:03:55 Anders Logg wrote:
> > > >>>>>>>>>>>> Will add it sometime this afternoon and then we can
> > > >>>>>>>>>>>> revisit the JIT compiler caching.
> > > >>>>>>>>>>> 
> > > >>>>>>>>>> It is confusing. Does the function 'preprocess' do anything
> > > >>>>>>>>>> that the old FormData class didn't? It would be easier to
> > > >>>>>>>>>> follow if Form just had a member function form_data() that
> > > >>>>>>>>>> computes and stores data (like it used to), or if Form had
> > > >>>>>>>>>> a 'preprocess' function. Having the function preprocess
> > > >>>>>>>>>> return a new form is really confusing.
> > > >>>>>>>>> 
> > > >>>>>>>> Which is the whole problem. By creating a new object, FormData
> > > >>>>>>>> is thrown away. The preprocessing should just compute some
> > > >>>>>>>> more data, just like we *don't* do
> > > >>>>>>>> 
> > > >>>>>>> As I explained, I thought it was better to have an explicit
> > > >>>>>>> call to preprocess since that makes it clear that one makes a
> > > >>>>>>> call to a function which may take some time to execute
> > > >>>>>>> (instead of just calling a member function which seems to just
> > > >>>>>>> return some data).
> > > >>>>>>> 
> > > >>>>>>> But as I say above: I added the caching back at some point
> > > >>>>>>> (maybe even the day after I removed it 2 years ago) so we
> > > >>>>>>> don't need to discuss why I removed it (as I realized myself I
> > > >>>>>>> shouldn't have removed it and added it back a long time ago).
> > > >>>>>>> 
> > > >>>>>>> What has me confused now is that the caching seems to be in
> > > >>>>>>> place but we still need the extra caching in FFC/DOLFIN and I
> > > >>>>>>> don't see why.
> > > >>>>>> 
> > > >>>>>> Now 'b' has been preprocessed, and has form data attached, but
> > > >>>>>> 'a' doesn't. Calling 'jit(a)' again, the code will never enter
> > > >>>>>> the 'else' part of the clause because 'a' never gets any form
> > > >>>>>> data. Johan has added some code FFC that attaches the form data
> > > >>>>>> of 'b' to 'a', but it is a bit clumsy.
> > > >>>>> 
> > > >>> I think the thing here is that form_data has always had a
> > > >>> preprocessed form. Someone (lets not point fingers!) thought that
> > > >>> was too much magic and added an
> > > >>> 
> > > >>> explicit need to call:
> > > >>>   form = preprocess(form)
> > > >>> 
> > > >>> introduced a cirucular dependency, as the return preprocessed form
> > > >>> need to know of its form_data, but the form_data already had a
> > > >>> reference to the preprocessed form. The latter is what I used in
> > > >>> the one line I altered.
> > > >> 
> > > >> No, it made the design cleaner since it makes clear something needs
> > > >> to happen to get the metadata: a call to preprocess.
> > 
> > Why is:

Just the first time. If a user need form_data it has to be generated anyhow, 
so why not on the fly? We do this other places.

Johan

> > > >>>>> form_data(). This will return the form_data. The preprocessed
> > > >>>>> form is attached to the form_data and this is what is passed to
> > > >>>>> the code generator. I am pretty sure this is what was there from
> > > >>>>> the beginning.
> > > >>>>> 
> > > >>>>> It is confusing to call:
> > > >>>>>   form = preprocess(form)
> > > >>>>> 
> > > >>>>>>>>>>>         debug("Form is already preprocessed. Not updating
> > > >>>>>>>>>>>         form data.") return form
> > > >>>>>>>>>>>     
> > > >>>>>>>>>>>     ...
> > > >>>>>>>>>>>     
> > > >>>>>>>>>>> And when I look at the blamelist (bzr annotate), it looks
> > > >>>>>>>>>>> like I added those lines, so I must have come to my senses
> > > >>>>>>>>>>> and added it back at some point (way back). So in
> > > >>>>>>>>>>> conclusion, calling preprocess() should not taking any
> > > >>>>>>>>>>> time.
> > > >>>>>>>>>>> 
> > > >>>>>>>>>>> What am I missing?
> > > >>>>>>

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 18:08:59 +0200

On 26 April 2011 18:07, Johan Hake <[email-masked]> wrote:

> > > > >>>>>>>> What was wrong with Martin's original design that
> necessitated
> > > > >>>>>>>> the change?
> > > > >>>>>>>

Why not just rename it to compute_form_data() then?

Martin

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 09:10:12 -0700

On Tuesday April 26 2011 09:01:35 Anders Logg wrote:
> > > > reference to the preprocessed form. The latter is what I used in the
> > > > one line I altered.
> > > 

In ffc.jit_form.

Your fix wont fix the circular dependency. 

We also need to remove form_data from the preprocessed form. This means that 
we need to return form_data from preprocess and maybe change its name to 
compute_form_data.

Johan

---

From: Martin Sandve Alnæs <[email-masked]>
Date: Tue, 26 Apr 2011 18:12:26 +0200

On 26 April 2011 18:10, Johan Hake <[email-masked]> wrote:

> to
> We also need to remove form_data from the preprocessed form.

Or just use a weakref.

This means that

Martin

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 18:16:40 +0200

>     we need to return form_data from preprocess and maybe change its name to

Why is the circular dependency a problem? Anyway, I'm thinking now the
cleanest design would be

  form.compute_form_data()
  form_data = form.form_data()
  preprocessed_form = form_data.preprocessed_form

Here, the preprocessed_form would not store form_data.

--
Anders

---

From: "Garth N. Wells" <[email-masked]>
Date: Tue, 26 Apr 2011 17:18:15 +0100

Agree.

Garth

---

From: Johan Hake <[email-masked]>
Date: Tue, 26 Apr 2011 09:24:02 -0700

On Tuesday April 26 2011 09:18:15 Garth N. Wells wrote:
> >>     > > > > > >>> On Tue, Apr 26, 2011 at 03:59:52PM +0100, Garth N.
> >>     > > > > > >>> Wells
> >>     
> >>     wrote:
> >>     > > > > > >>>>> On Tue, Apr 26, 2011 at 03:45:22PM +0100, Garth N.
> >>     > > > > > >>>>> Wells
> >>     
> >>     wrote:
> >>     > > > > > >>>>>>> On Tue, Apr 26, 2011 at 02:00:50PM +0200, Anders
> >>     > > > > > >>>>>>> Logg
> >>     
> >>     wrote:
> >>     > > > > > >>>>>>>> Will add it sometime this afternoon and then we
> >>     > > > > > >>>>>>>> can revisit the JIT compiler caching.
> >>     > > > > > >>>>>>> 
> >>     > > > > > >>>>>> It is confusing. Does the function 'preprocess' do
> >>     
> >>     anything
> >>     
> >>     > > > > > >>>>>> that the old FormData class didn't? It would be
> >>     > > > > > >>>>>> easier to follow if Form just had a member
> >>     > > > > > >>>>>> function form_data() that computes and stores data
> >>     > > > > > >>>>>> (like it used to), or if Form had a 'preprocess'
> >>     > > > > > >>>>>> function. Having the function preprocess return a
> >>     > > > > > >>>>>> new form is really confusing.
> >>     > > > > > >>>>> 
> >>     > > > > > >>>> Which is the whole problem. By creating a new object,
> >>     
> >>     FormData
> >>     
> >>     > > > > > >>>> is thrown away. The preprocessing should just compute
> >>     > > > > > >>>> some more data, just like we *don't* do
> >>     > > > > > >>>> 
> >>     > > > > > >>> As I explained, I thought it was better to have an
> >>     > > > > > >>> explicit call to preprocess since that makes it clear
> >>     > > > > > >>> that one makes a call to a function which may take
> >>     > > > > > >>> some time to execute (instead of just calling a
> >>     > > > > > >>> member function which seems to
> >>     
> >>     just
> >>     
> >>     > > > > > >>> return some data).
> >>     > > > > > >>> 
> >>     > > > > > >>> But as I say above: I added the caching back at some
> >>     > > > > > >>> point (maybe even the day after I removed it 2 years
> >>     > > > > > >>> ago) so we don't need to discuss why I removed it (as
> >>     > > > > > >>> I realized myself
> >>     
> >>     I
> >>     
> >>     > > > > > >>> What has me confused now is that the caching seems to
> >>     > > > > > >>> be in place but we still need the extra caching in
> >>     > > > > > >>> FFC/DOLFIN and I don't see why.
> >>     > > > > > >> 
> >>     > > > > > >> Now 'b' has been preprocessed, and has form data
> >>     > > > > > >> attached, but 'a' doesn't. Calling 'jit(a)' again, the
> >>     > > > > > >> code will never enter the 'else' part of the clause
> >>     > > > > > >> because 'a' never gets any form data. Johan has added
> >>     > > > > > >> some code FFC that attaches the form
> >>     
> >>     data
> >>     
> >>     > > > > preprocessed form. Someone (lets not point fingers!) thought
> >>     > > > > that was too much magic and added an
> >>     > > > > 
> >>     > > > > explicit need to call:
> >>     > > > >   form = preprocess(form)
> >>     > > > > 
> >>     > > > > in jit_compiler(). This made the design more complicated and
> >>     > > > > also introduced a cirucular dependency, as the return
> >>     > > > > preprocessed form need to know of its form_data, but the
> >>     > > > > form_data already had a reference to the preprocessed form.
> >>     > > > > The latter is what I used in
> >>     
> >>     the
> >>     
> >>     > > > > one line I altered.
> >>     > > > 
> >>     > I've fixed the bug now in preprocess.py (attaching to both forms).
> >>     > Does that help?
> >>     
> >>     In ffc.jit_form.
> >>     
> >>     we need to return form_data from preprocess and maybe change its
> >>     name to compute_form_data.
> >>     
> >>     Johan
> > 

Go for it! We would then also avoid problems with ciruclar dep.

Still think form.compute_form_data() line is superflous ;)

Johan

---

From: Anders Logg <[email-masked]>
Date: Tue, 26 Apr 2011 18:40:14 +0200

> Go for it! We would then also avoid problems with ciruclar dep.

Great! Will fix soon.

> Still think form.compute_form_data() line is superflous ;)

I foresee we will have another long discussion in a year or two and we
will then end up removing compute_form_data... :-)

--
Anders