Hello,

This patch allows us to remove all the calls to commutex.  Commutex is
a convenience function left over from the pre-type-witness days.

Thomas DuBuisson and I were looking at exponetial merge problems via
profiler output and we found it terribly confusing that commute and
commutex were implemented in terms of each other in the default
implementation.  We had a concern that the hang could potentially be
caused by mutualy recursive definitions.  Removing one of the two
makes it easier to be certain that no infinite mutual recursions are
causing the hangs.  Note: I haven't yet been able to test these
changes on a hanging merge so I don't know if they have any impact on
that in practice.

toCommute and fromCommute are not the best function names, but I think
a future refactor can get rid of both of them or give them better
names.  I've also tried to make their types sufficiently general such
that they may be able to transform the type of commuteFL, although I
haven't actually tried it.

I had to refactor the tests a bit also.

Thanks,
Jason

Sat Nov 14 23:03:56 PST 2009  Jason Dagit <dagit@codersbase.com>
  * remove commutex

Attachments

• remove-commutex.dpatch
• unnamed

By the way, this patch bundle depends on patch61, but I forgot to include
those patches in this bundle, and darcs had no way to detect that
dependency.

Sorry for any inconvenience,
Jason

Attachments

• unnamed

On Sun, 15 Nov 2009, Jason Dagit wrote:

> toCommute and fromCommute are not the best function names, but I think
> a future refactor can get rid of both of them or give them better
> names.  I've also tried to make their types sufficiently general such
> that they may be able to transform the type of commuteFL, although I
> haven't actually tried it.

A little while ago I started adding some general infrastructure for 
handling commutes between different types of patches in 
Darcs.Patch.Permutations - in particular there's

type CommuteFn p1 p2 = FORALL(x y) (p1 :> p2) C(x y) -> Maybe ((p2 :> p1) 
C(x y))

I think it would make sense to combine this stuff with your toCommute and 
fromCommute code, and put it in an appropriate module (perhaps a new one).

Cheers,

Ganesh

On Sun, Nov 15, 2009 at 8:15 AM, Ganesh Sittampalam <ganesh@earth.li> wrote:

> On Sun, 15 Nov 2009, Jason Dagit wrote:
>
>  toCommute and fromCommute are not the best function names, but I think
>> a future refactor can get rid of both of them or give them better
>> names.  I've also tried to make their types sufficiently general such
>> that they may be able to transform the type of commuteFL, although I
>> haven't actually tried it.
>>
>
> A little while ago I started adding some general infrastructure for
> handling commutes between different types of patches in
> Darcs.Patch.Permutations - in particular there's
>
> type CommuteFn p1 p2 = FORALL(x y) (p1 :> p2) C(x y) -> Maybe ((p2 :> p1)
> C(x y))
>
> I think it would make sense to combine this stuff with your toCommute and
> fromCommute code, and put it in an appropriate module (perhaps a new one).
>

Ganesh,

It's unclear to me:  Were you going to apply this?  I see it hasn't made it
into darcs.net yet, and the status is "amend-requested" but I thought you
were commenting on future work above.  Did you feel that I need to include a
usage of CommuteFn at the same time?  If so, I think we could use a
different set of patches for that.  Take for example this output:
$ find src/ | xargs grep "type.*Commute"
src/Darcs/Patch/Commute.lhs:type CommuteFunction = FORALL(x y) (Patch :<
Patch) C(x y) -> Perhaps ((Patch :< Patch) C(x y))
src/Darcs/Patch/Commute.lhs:type MaybeCommute = FORALL(x y) (Patch :< Patch)
C(x y) -> Maybe ((Patch :< Patch) C(x y))
src/Darcs/Patch/Permutations.hs:type CommuteFn p1 p2 = FORALL(x y) (p1 :>
p2) C(x y) -> Maybe ((p2 :> p1) C(x y))
src/Darcs/Patch/Prim.lhs:type CommuteFunction = FORALL(x y) (Prim :< Prim)
C(x y) -> Perhaps ((Prim :< Prim) C(x y))

It looks to me like there is a bit of thinking to be done about unifying the
above and refactoring it properly.  I agree it needs to happen, but I'd
prefer it to be independent of the removal of commutex (mainly because that
bit has already been done, and the refactor of the types would delay it
getting into darcs).

What do you think?

Jason

Attachments

• unnamed

On Tue, 24 Nov 2009, Jason Dagit wrote:

> On Sun, Nov 15, 2009 at 8:15 AM, Ganesh Sittampalam <ganesh@earth.li> wrote:
>
>> On Sun, 15 Nov 2009, Jason Dagit wrote:
>>
>>  toCommute and fromCommute are not the best function names, but I think
>>> a future refactor can get rid of both of them or give them better
>>> names.  I've also tried to make their types sufficiently general such
>>> that they may be able to transform the type of commuteFL, although I
>>> haven't actually tried it.
>>>
>>
>> A little while ago I started adding some general infrastructure for
>> handling commutes between different types of patches in
>> Darcs.Patch.Permutations - in particular there's
>>
>> type CommuteFn p1 p2 = FORALL(x y) (p1 :> p2) C(x y) -> Maybe ((p2 :> p1)
>> C(x y))
>>
>> I think it would make sense to combine this stuff with your toCommute and
>> fromCommute code, and put it in an appropriate module (perhaps a new one).
>>
>
> Ganesh,
>
> It's unclear to me:  Were you going to apply this?  I see it hasn't made it
> into darcs.net yet, and the status is "amend-requested"

it's "review-in-progress" - I've had a quick look but not finished yet.

> but I thought you
> were commenting on future work above.  Did you feel that I need to include a
> usage of CommuteFn at the same time?

No, but I didn't really like the toCommute/fromCommute names at all, so 
was erring towards asking that they be changed before it's applied. Hadn't 
thought of good alternatives yet which is part of the reason I haven't 
finished the review yet.

> It looks to me like there is a bit of thinking to be done about unifying the
> above and refactoring it properly.  I agree it needs to happen, but I'd
> prefer it to be independent of the removal of commutex (mainly because that
> bit has already been done, and the refactor of the types would delay it
> getting into darcs).
>
> What do you think?

Yes, I agree with that part.

Ganesh

On Tue, Nov 24, 2009 at 9:36 PM, Ganesh Sittampalam <ganesh@earth.li> wrote:

> On Tue, 24 Nov 2009, Jason Dagit wrote:
>
>  On Sun, Nov 15, 2009 at 8:15 AM, Ganesh Sittampalam <ganesh@earth.li>
>> wrote:
>>
>>  On Sun, 15 Nov 2009, Jason Dagit wrote:
>>>
>>>  toCommute and fromCommute are not the best function names, but I think
>>>
>>>> a future refactor can get rid of both of them or give them better
>>>> names.  I've also tried to make their types sufficiently general such
>>>> that they may be able to transform the type of commuteFL, although I
>>>> haven't actually tried it.
>>>>
>>>>
>>> A little while ago I started adding some general infrastructure for
>>> handling commutes between different types of patches in
>>> Darcs.Patch.Permutations - in particular there's
>>>
>>> type CommuteFn p1 p2 = FORALL(x y) (p1 :> p2) C(x y) -> Maybe ((p2 :> p1)
>>> C(x y))
>>>
>>> I think it would make sense to combine this stuff with your toCommute and
>>> fromCommute code, and put it in an appropriate module (perhaps a new
>>> one).
>>>
>>>
>> Ganesh,
>>
>> It's unclear to me:  Were you going to apply this?  I see it hasn't made
>> it
>> into darcs.net yet, and the status is "amend-requested"
>>
>
> it's "review-in-progress" - I've had a quick look but not finished yet.


Oh, maybe I misremembered it.  I checked the status then hopped on transit
where I wrote the email (without access to the tracker).


>
>
>  but I thought you
>> were commenting on future work above.  Did you feel that I need to include
>> a
>> usage of CommuteFn at the same time?
>>
>
> No, but I didn't really like the toCommute/fromCommute names at all, so was
> erring towards asking that they be changed before it's applied. Hadn't
> thought of good alternatives yet which is part of the reason I haven't
> finished the review yet.


I struggled with the names too.  They transform the argument tuples.
Similar to curry/uncurry.  So the better names might be, forwardCommute and
reverseCommute.  Or, toForwardCommute/toReverseCommute, but those are
getting pretty long.

I think eventually we can make all the tuples go the same way.  Which would
probably reduce some duplication.  Having a 'flip' function for this might
be nice too.  Just some ideas to help you get unstuck on this.

Thanks,
Jason

Attachments

• unnamed

On Tue, 24 Nov 2009, Jason Dagit wrote:

>> No, but I didn't really like the toCommute/fromCommute names at all, so was
>> erring towards asking that they be changed before it's applied. Hadn't
>> thought of good alternatives yet which is part of the reason I haven't
>> finished the review yet.
>
>
> I struggled with the names too.  They transform the argument tuples.
> Similar to curry/uncurry.  So the better names might be, forwardCommute and
> reverseCommute.  Or, toForwardCommute/toReverseCommute, but those are
> getting pretty long.

Sorry for the delay on this. I think the rest of the patch is fine and a 
good thing, but please do change the names. Either of your suggestions if 
fine, I think I prefer toForward and toReverse (could abbreviate to toFwd 
and toRev?). Also I suggest dropping the comment about them being to help 
the refactor from commutex, because once commutex has gone they are 
meaningless.

Ganesh

Ganesh,

I believe these are the changes you requested.

Thanks,
Jason

Sat Nov 14 23:03:56 PST 2009  Jason Dagit <dagit@codersbase.com>
  * remove commutex

Mon Dec  7 14:00:00 PST 2009  Jason Dagit <dagit@codersbase.com>
  * remove no longer meaningful comments

Mon Dec  7 14:00:31 PST 2009  Jason Dagit <dagit@codersbase.com>
  * rename toCommute to toFwd and fromCommute to toRev

Attachments

• remove-commutex.dpatch
• unnamed

On Mon, 7 Dec 2009, Jason Dagit wrote:

> Mon Dec  7 14:00:31 PST 2009  Jason Dagit <dagit@codersbase.com>
>  * rename toCommute to toFwd and fromCommute to toRev

Doh, sorry, I was unclear; I meant "toFwdCommute and toRevCommute", rather 
than literally "toFwd and toRev". I think the latter is too confusing.

Ganesh

Hopefully it's right this time.

And hopefully it goes to patch65 instead of issue65.

Thanks,
Jason


Sat Nov 14 23:03:56 PST 2009  Jason Dagit <dagit@codersbase.com>
  * remove commutex

Mon Dec  7 14:00:00 PST 2009  Jason Dagit <dagit@codersbase.com>
  * remove no longer meaningful comments

Mon Dec  7 14:52:16 PST 2009  Jason Dagit <dagit@codersbase.com>
  * rename toCommute to toFwdCommute and fromCommute to toRevCommute

Attachments

• remove-commutex.dpatch
• unnamed

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-5b6427c2f12e0404d3640f2492e226b73baa6d85

This patch bundle (with 3 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-5b6427c2f12e0404d3640f2492e226b73baa6d85

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-bfc9e44ac20bf524cbb49d97cf0b6251cff7ac3e