Rules system optimization

[nobrakal]

Hi,

I have been working recently with fusion, and it made me think about some optimizations that can be made to the current system for Algebra.Graph.

This:

1. Change to good producers vertices, edges (ie. they are now expressed directly with buildR)... Meaning that foldg e v o c . vertices xs is optimized to a single list traversal without a custom a rule.

2. bind is also a good producer, so I added its buildR definition directly.

3. star is now (in addition to be a good producer of graph), a good consumer of lists (meaning that star . map f is also optimized`).

4. The foldr1Safe rule is now unneeded, I removed it.

5. I added INLINE pragmas to functions expressed with foldg.

All the previous tests are passing, and the benchmarking suite is happy.

Questions

I have some questions:

• Can we expose buildR ? Maybe moving it in Algebra.Graph.Internal, but exposing such a function allows the end-user to define its own function using buildR and benefit from fusion using foldg/buildR.

• The whole thing needs documentation. I like the "good producer" / "good consumer" vocabulary (stolen from the GHC manual). A good consumer is a function that can be fused with a good producer, and a good producer is a function that can be fused with a good consumer. Then we can tag all concerned functions.

• Because of the use of buildR, we need rewrite rules to be enabled (either, some foldg Empty Vertex Overlay Connect will not be optimized). There is no problem for the library itself, but we should add a warning about it somewhere.

[snowleopard]

@nobrakal Thanks for the PR! I like the overall goal: introducing the vocabulary of "good" producers and consumers and making sure that all exported functions are as good as possible!

It would, of course, be nice if users could write their own good producers and consumers.

My concern is that users who don't know (and perhaps don't want to know) about rewrite rules will be exposed to them if we start exporting functions like buildR. It would be great if the user could write simple code like overlays . map vertex and get an efficient implementation (i.e. a good producer).

I would have hoped for the notion of "goodness" to be composable, i.e. if map preservers goodness in the world of lists, and overlays is a good consumer of lists and a good producer of graphs, then their composition is automatically a good consumer of lists and a good producer of graphs. Is there any hope for achieving something like this?

If we make goodness compositional we will never run out of goodness! :)

[snowleopard]

P.S.: I guess what I'm after is a "good category" :-) The identity function id is hopefully both a good consumer and producer, and I'd like . to preserve goodness too!

[nobrakal]

I added some comments, many functions are good producers and good consumers :D (biclique even ended to be a good consumer of both its arguments and a good producer !)

My concern is that users who don't know (and perhaps don't want to know) about rewrite rules ...

That why I propose to export it from Algebra.Graph.Internal (like build is exported from GHC.Base for lists). Then only people that are searching for it will find it :)

I would have hoped for the notion of "goodness" to be composable ...

I think that, strictly speaking, the notion of "goodness" is composable. The only problem here for me is that overlays is a good consumer of lists, and not of list of graphs. An other way to say is: because buildR abstracts all the constructors of graphs, thus any good producer should be expressed "in one go". For overlays . map vertex to be a good producer, overlays should know the previous map vertex to abstract the Vertex constructor.

[snowleopard]

I added some comments, many functions are good producers and good consumers :D (biclique even ended to be a good consumer of both its arguments and a good producer !)

@nobrakal Great! :)

That why I propose to export it from Algebra.Graph.Internal (like build is exported from GHC.Base for lists). Then only people that are searching for it will find it :)

OK, let's do that. I'd prefer to export it with a simpler name build or buildg (instead of buildR).

The only problem here for me is that overlays is a good consumer of lists, and not of list of graphs.

Oh, I see. That's more tricky than I thought -- there are layers of goodness!

[nobrakal]

OK, let's do that. I'd prefer to export it with a simpler name build or buildg (instead of buildR).

I said it before trying, but buildR needs to know the graphs constructors, so we cannot put it in Algebra.Graph.Internal.

[snowleopard]

@nobrakal In this case, let's export it as buildg from Algebra.Graph.

[nobrakal]

I addressed your comments. More generally, I don't know the amount of documentation we need to add... There are many things to say :)

[snowleopard]

@nobrakal What about removing the Foldg synonym? I've suggested a comment that might work if you simply inline Foldg into the definition of buildg.

[snowleopard]

@nobrakal I had a thorough look through the changes and left a bunch of comments, most of which should be easy to address. Hope we can merge this soon!

[nobrakal]

Thank you very much for your detailed review ! I addressed "simple" issues and will come back soon to more difficult ones ^^

[snowleopard]

@nobrakal Thanks for addressing my comments!

By the way, it looks like there are some merge conflicts. Please rebase.

[nobrakal]

@snowleopard I do not understand: it seems I am up-to-date with master. A local merge passes without problem on my machine, and even github seems happy with the PR.

[snowleopard]

@nobrakal Oops, looks like a GitHub bug. I see

This branch cannot be rebased due to conflicts
Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

when "Rebase and merge" is selected. If I switch to "Squash and merge", which is more appropriate in this case anyway, everything gets green!

[snowleopard]

@nobrakal OK, I think we can merge this if you are ready!

[nobrakal]

I just found a new good consumer! Except for this, all seems green for a merge :)

[snowleopard]

@nobrakal Merged! Many thanks for this awesome improvement :-)