The libraries proposal process and containers
Milan just pushed all the following patches directly to the containers repo, and closed all the tickets associated with the effort. * Fix warnings in Data.Map and Data.Set. * Finish the started worker/wrapper transformation. * Merge all the OPTIONS and LANGUAGE module pragmas. * Remove most INLINE from Map, Set, IntMap and IntSet. * Comment tests and benchmarks on foldlWithKey' * Worker/wrapper transformation for Data.IntSet. * Compile only the benchmark source, not the Data/*.hs. * Add criterion-based benchmark for IntSet.hs * Add a testsuite for Data.IntSet. * Further improve Data.Set balance function * Further improve Data.Map balance function * Changing delta to 3 in Data.Set. * Changing delta to 3 in Data.Map. * Correct Data.Set Arbitrary instance never to return unbalanced trees. * Correct Data.Map Arbitrary instance never to return unbalanced trees. * Improve Data.Set benchmark. * Improve benchmark infrastructure and Data.Map benchmark * Improve the performance of Data.Set balance function * Improve the performance of Data.Map balance function. * Improve performance of Data.Set union and difference operations * Improve performance of Data.Map union* and difference operations * Make the Set store the elements evaluated (bang added). Have *any* of these patches been proposed for review? containers is a critical library, and under libraries@ maintainance. This is not what you do prior to a release. Ian, Simon M.: did you get a chance to sign off on this? -- Don
Hi, I am terribly sorry if I did it wrong. I posted the tickets to libraries, there were some discussion about some of them. After sorting the INLINE => INLINABLE issue (I agree that in a hurry) I pushed all the tickets (I was working on top of my repo). Once again, if I should have waited more, sorry. Rollback if I overstepped my authority. Sorry, Milan
Milan just pushed all the following patches directly to the containers repo, and closed all the tickets associated with the effort.
* Fix warnings in Data.Map and Data.Set. * Finish the started worker/wrapper transformation. * Merge all the OPTIONS and LANGUAGE module pragmas. * Remove most INLINE from Map, Set, IntMap and IntSet. * Comment tests and benchmarks on foldlWithKey' * Worker/wrapper transformation for Data.IntSet. * Compile only the benchmark source, not the Data/*.hs. * Add criterion-based benchmark for IntSet.hs * Add a testsuite for Data.IntSet. * Further improve Data.Set balance function * Further improve Data.Map balance function * Changing delta to 3 in Data.Set. * Changing delta to 3 in Data.Map. * Correct Data.Set Arbitrary instance never to return unbalanced trees. * Correct Data.Map Arbitrary instance never to return unbalanced trees. * Improve Data.Set benchmark. * Improve benchmark infrastructure and Data.Map benchmark * Improve the performance of Data.Set balance function * Improve the performance of Data.Map balance function. * Improve performance of Data.Set union and difference operations * Improve performance of Data.Map union* and difference operations * Make the Set store the elements evaluated (bang added).
Have *any* of these patches been proposed for review? containers is a critical library, and under libraries@ maintainance. This is not what you do prior to a release.
Ian, Simon M.: did you get a chance to sign off on this?
-- Don
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
Firstly, you shouldn't be committing into the containers library from your working copy. You're not the maintainer. Secondly, if you have patches to propose, you have to: * prepare a single patch for review by this list. * propose it, via a ticket, and attach the patch. * send the patch to this list for comments. * after the time is up, if there was consensus, you can recommend to Ian it be pushed. Never, ever just push your private code directly to the containers library. So, for example, you should have a proposal to rewrite all the balance functions, and a patch that shows what you change. If we all agree, then Ian or Simon can commit it. Ian : can you fix this? -- Don fox:
Hi,
I am terribly sorry if I did it wrong.
I posted the tickets to libraries, there were some discussion about some of them.
After sorting the INLINE => INLINABLE issue (I agree that in a hurry) I pushed all the tickets (I was working on top of my repo).
Once again, if I should have waited more, sorry. Rollback if I overstepped my authority.
Sorry, Milan
Milan just pushed all the following patches directly to the containers repo, and closed all the tickets associated with the effort.
* Fix warnings in Data.Map and Data.Set. * Finish the started worker/wrapper transformation. * Merge all the OPTIONS and LANGUAGE module pragmas. * Remove most INLINE from Map, Set, IntMap and IntSet. * Comment tests and benchmarks on foldlWithKey' * Worker/wrapper transformation for Data.IntSet. * Compile only the benchmark source, not the Data/*.hs. * Add criterion-based benchmark for IntSet.hs * Add a testsuite for Data.IntSet. * Further improve Data.Set balance function * Further improve Data.Map balance function * Changing delta to 3 in Data.Set. * Changing delta to 3 in Data.Map. * Correct Data.Set Arbitrary instance never to return unbalanced trees. * Correct Data.Map Arbitrary instance never to return unbalanced trees. * Improve Data.Set benchmark. * Improve benchmark infrastructure and Data.Map benchmark * Improve the performance of Data.Set balance function * Improve the performance of Data.Map balance function. * Improve performance of Data.Set union and difference operations * Improve performance of Data.Map union* and difference operations * Make the Set store the elements evaluated (bang added).
Have *any* of these patches been proposed for review? containers is a critical library, and under libraries@ maintainance. This is not what you do prior to a release.
Ian, Simon M.: did you get a chance to sign off on this?
-- Don
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
Hi,
Firstly, you shouldn't be committing into the containers library from your working copy. You're not the maintainer.
Secondly, if you have patches to propose, you have to:
* prepare a single patch for review by this list. * propose it, via a ticket, and attach the patch. * send the patch to this list for comments. * after the time is up, if there was consensus, you can recommend to Ian it be pushed.
Never, ever just push your private code directly to the containers library.
So, for example, you should have a proposal to rewrite all the balance functions, and a patch that shows what you change. If we all agree, then Ian or Simon can commit it.
Sorry. The rewrite process of the balance functions was in tickets 4311 and 4312. Once again, I am sorry, everyone. I was feeling urged by the INLINE => INLINABLE stuff and committed without thinking. Milan
Ian : can you fix this?
-- Don
fox:
Hi,
I am terribly sorry if I did it wrong.
I posted the tickets to libraries, there were some discussion about some of them.
After sorting the INLINE => INLINABLE issue (I agree that in a hurry) I pushed all the tickets (I was working on top of my repo).
Once again, if I should have waited more, sorry. Rollback if I overstepped my authority.
Sorry, Milan
Milan just pushed all the following patches directly to the containers repo, and closed all the tickets associated with the effort.
* Fix warnings in Data.Map and Data.Set. * Finish the started worker/wrapper transformation. * Merge all the OPTIONS and LANGUAGE module pragmas. * Remove most INLINE from Map, Set, IntMap and IntSet. * Comment tests and benchmarks on foldlWithKey' * Worker/wrapper transformation for Data.IntSet. * Compile only the benchmark source, not the Data/*.hs. * Add criterion-based benchmark for IntSet.hs * Add a testsuite for Data.IntSet. * Further improve Data.Set balance function * Further improve Data.Map balance function * Changing delta to 3 in Data.Set. * Changing delta to 3 in Data.Map. * Correct Data.Set Arbitrary instance never to return unbalanced trees. * Correct Data.Map Arbitrary instance never to return unbalanced trees. * Improve Data.Set benchmark. * Improve benchmark infrastructure and Data.Map benchmark * Improve the performance of Data.Set balance function * Improve the performance of Data.Map balance function. * Improve performance of Data.Set union and difference operations * Improve performance of Data.Map union* and difference operations * Make the Set store the elements evaluated (bang added).
Have *any* of these patches been proposed for review? containers is a critical library, and under libraries@ maintainance. This is not what you do prior to a release.
Ian, Simon M.: did you get a chance to sign off on this?
-- Don
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
Hi, to clatify what I pushed: - 4277 with discussion period over (dons) - 4279 with discussion period over (dons) - 4280 with discussion period over (tibbe) - 4311 without explicit period (my mistake), there were some reactions, I created v2 of the patch - 4312 without explicit period (my mistake), there were some reactions, I created v2 of the patch - 4333 without explicit period (my mistake) This is just consistency with 4277, 4278 and 4280. - INLINE => INLINABLE issue which was being sorted out at libraries@ - some trivial changes like warnings and rearranging OPTIONS and LANGUAGE. All these things do not touch API. The performance patches have benchmark results on them. I pushed hastily as I wanted to sort out the INLINE => INLINABLE issue and I was working on the top of my repo with all these performance changes. I will not do it again. Sorry, Milan
Hi,
I am terribly sorry if I did it wrong.
I posted the tickets to libraries, there were some discussion about some of them.
After sorting the INLINE => INLINABLE issue (I agree that in a hurry) I pushed all the tickets (I was working on top of my repo).
Once again, if I should have waited more, sorry. Rollback if I overstepped my authority.
Sorry, Milan
Milan just pushed all the following patches directly to the containers repo, and closed all the tickets associated with the effort.
* Fix warnings in Data.Map and Data.Set. * Finish the started worker/wrapper transformation. * Merge all the OPTIONS and LANGUAGE module pragmas. * Remove most INLINE from Map, Set, IntMap and IntSet. * Comment tests and benchmarks on foldlWithKey' * Worker/wrapper transformation for Data.IntSet. * Compile only the benchmark source, not the Data/*.hs. * Add criterion-based benchmark for IntSet.hs * Add a testsuite for Data.IntSet. * Further improve Data.Set balance function * Further improve Data.Map balance function * Changing delta to 3 in Data.Set. * Changing delta to 3 in Data.Map. * Correct Data.Set Arbitrary instance never to return unbalanced trees. * Correct Data.Map Arbitrary instance never to return unbalanced trees. * Improve Data.Set benchmark. * Improve benchmark infrastructure and Data.Map benchmark * Improve the performance of Data.Set balance function * Improve the performance of Data.Map balance function. * Improve performance of Data.Set union and difference operations * Improve performance of Data.Map union* and difference operations * Make the Set store the elements evaluated (bang added).
Have *any* of these patches been proposed for review? containers is a critical library, and under libraries@ maintainance. This is not what you do prior to a release.
Ian, Simon M.: did you get a chance to sign off on this?
-- Don
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
| All these things do not touch API. The performance patches have | benchmark results on them. | | I pushed hastily as I wanted to sort out the INLINE => INLINABLE issue | and I was working on the top of my repo with all these performance | changes. Let's not go overboard here. OK, Milan didn't do the Right Thing, but: * It's GREAT that he has taken the time to improve the containers library. It's used by lots of people, so the benefits will be widely felt. But it takes painstaking work to find make solid performance improvements. Thank you Milan! We need more people like you. * Most if not all of the changes relate to performance and not API. And the last patch (itself pushed over-hastily by Simon M) had some negative impact on code size that we've been keen to see rectified in time for the GHC release. (There is still time; we are only at release candidate stage.) * The maintainer of 'containers' is libraries@haskell.org, so I don't think it's true to say that Milan isn't the maintainer. As I understand the protocol for such packages, one makes a proposal, and if there is no dissent, one pushes it. I am agnostic about how to proceed from here. We could roll back all the patches (Milan's and the previous one that Simon M pushed). Or we could move on from here. So don't feel bad, Milan. Incidentally, do you have any more performance improvement in the pipeline? Simon
| All these things do not touch API. The performance patches have | benchmark results on them. | | I pushed hastily as I wanted to sort out the INLINE => INLINABLE issue | and I was working on the top of my repo with all these performance | changes.
Let's not go overboard here. OK, Milan didn't do the Right Thing, but:
* It's GREAT that he has taken the time to improve the containers library. It's used by lots of people, so the benefits will be widely felt. But it takes painstaking work to find make solid performance improvements. Thank you Milan! We need more people like you.
* Most if not all of the changes relate to performance and not API. And the last patch (itself pushed over-hastily by Simon M) had some negative impact on code size that we've been keen to see rectified in time for the GHC release. (There is still time; we are only at release candidate stage.)
* The maintainer of 'containers' is libraries@haskell.org, so I don't think it's true to say that Milan isn't the maintainer. As I understand the protocol for such packages, one makes a proposal, and if there is no dissent, one pushes it.
I am agnostic about how to proceed from here. We could roll back all the patches (Milan's and the previous one that Simon M pushed). Or we could move on from here.
So don't feel bad, Milan. Incidentally, do you have any more performance improvement in the pipeline?
No, the performance patches are currently all in. There are some API changes (like the folds) I am thinking about, but the performance is currently done. More performance patches will come if/when we have SPECIALISABLE. Thanks, Milan
Hi Milan,
as you admitted, some of the commits were overly hasted, but everyone
makes mistakes, so don't worry. You'll know next time. The tickets
for which the discussion period was over were completely fine. You
don't have to ask Ian or any of the Simons if there were no issues
raised and you have properly validated.
For #4311, #4312, #4333, you admitted skipping the proper process, but
I think we don't need to revert anything. Minor changes or
non-API-visible changes (like adding a benchmark suite) are fine, too.
The only real issue I think is the INLINE vs. INLINABLE change which I
didn't feel was resolved. There was some disagreement about whether
the bloat is worth it (since INLINABLE didn't not improve
performance). The discussion seemed to move towards the compromise
you just committed (INLINABLE for now, think about INLINE features for
after 7.0.1), but it wasn't clear that this was the agreed comprise.
In the future: If there is no clear consensus just send an email
summarising the discussion (as you understand it) and state what
appears to be the consensus (or propose one). This will hopefully
focus the discussion and find a clear consensus.
This is similar to the process we have in place for platform proposals
and I think it's good one.
Anyway, we make mistakes -- we learn from them.
Keep up the good work!
/ Thomas
On 24 September 2010 18:33, Milan Straka
Hi,
to clatify what I pushed:
- 4277 with discussion period over (dons) - 4279 with discussion period over (dons) - 4280 with discussion period over (tibbe) - 4311 without explicit period (my mistake), there were some reactions, I created v2 of the patch - 4312 without explicit period (my mistake), there were some reactions, I created v2 of the patch - 4333 without explicit period (my mistake) This is just consistency with 4277, 4278 and 4280. - INLINE => INLINABLE issue which was being sorted out at libraries@ - some trivial changes like warnings and rearranging OPTIONS and LANGUAGE.
All these things do not touch API. The performance patches have benchmark results on them.
I pushed hastily as I wanted to sort out the INLINE => INLINABLE issue and I was working on the top of my repo with all these performance changes.
I will not do it again.
Sorry, Milan
Hi,
I am terribly sorry if I did it wrong.
I posted the tickets to libraries, there were some discussion about some of them.
After sorting the INLINE => INLINABLE issue (I agree that in a hurry) I pushed all the tickets (I was working on top of my repo).
Once again, if I should have waited more, sorry. Rollback if I overstepped my authority.
Sorry, Milan
Milan just pushed all the following patches directly to the containers repo, and closed all the tickets associated with the effort.
* Fix warnings in Data.Map and Data.Set. * Finish the started worker/wrapper transformation. * Merge all the OPTIONS and LANGUAGE module pragmas. * Remove most INLINE from Map, Set, IntMap and IntSet. * Comment tests and benchmarks on foldlWithKey' * Worker/wrapper transformation for Data.IntSet. * Compile only the benchmark source, not the Data/*.hs. * Add criterion-based benchmark for IntSet.hs * Add a testsuite for Data.IntSet. * Further improve Data.Set balance function * Further improve Data.Map balance function * Changing delta to 3 in Data.Set. * Changing delta to 3 in Data.Map. * Correct Data.Set Arbitrary instance never to return unbalanced trees. * Correct Data.Map Arbitrary instance never to return unbalanced trees. * Improve Data.Set benchmark. * Improve benchmark infrastructure and Data.Map benchmark * Improve the performance of Data.Set balance function * Improve the performance of Data.Map balance function. * Improve performance of Data.Set union and difference operations * Improve performance of Data.Map union* and difference operations * Make the Set store the elements evaluated (bang added).
Have *any* of these patches been proposed for review? containers is a critical library, and under libraries@ maintainance. This is not what you do prior to a release.
Ian, Simon M.: did you get a chance to sign off on this?
-- Don
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
-- Push the envelope. Watch it bend.
Don Stewart wrote:
Have *any* of these patches been proposed for review?
Hey, chill out dons! Yes, most of them have been proposed for review, and accepted.
containers is a critical library,
I hope not. As has been demonstrated recently, containers still suffers from correctness bugs and performance lacks. No-one should treat it as critical infrastructure yet. Indeed the source code itself still claims "Stability: provisional".
and under libraries@ maintainance.
In general, I think assigning maintenance (of any library, not specifically this one) to libraries@h.o is rather suboptimal. In my view, it is better for a library to have a dedicated individual person who cares about it, than a committee of random strangers who are only focussed on fixing their own problem-du-jour, but lack the wider view. In this case, Milan is not just a random stranger - he has a paper accepted at the Haskell Symposium, to be presented in a couple of days time, about these very patches! That sounds awfully like a "maintainer who cares" to me. Regards, Malcolm
participants (5)
-
Don Stewart -
Malcolm Wallace -
Milan Straka -
Simon Peyton-Jones -
Thomas Schilling