Hi, I noticed IntSet.findMax is defined (more or less) as "fst . maxView", but maxView allocates thunks on its way down to the max leaf, only to have findMax throw them away. Same for IntMap. I wrote a patch to fix this. I didn't test it for speed (couldn't think of a good test. I did test for correctness.) However, I looked at the core and the old version did indeed have "let"s in the loop. The new version does not. Also I changed the API a bit. Previously, IntMap.findMin only returned the bound value, not the key. Data.Map returns the pair, so I changed IntMap to do the same. (This seems like common sense to me.) Patch is attached. Can you tell I'm using this library a lot lately? Scott
Ross, are you able, as containers maintainer, to act as the designated reviewer of Scott's IntMap patches? -- Don sedillard:
Hi,
I noticed IntSet.findMax is defined (more or less) as "fst . maxView", but maxView allocates thunks on its way down to the max leaf, only to have findMax throw them away. Same for IntMap. I wrote a patch to fix this. I didn't test it for speed (couldn't think of a good test. I did test for correctness.) However, I looked at the core and the old version did indeed have "let"s in the loop. The new version does not. Also I changed the API a bit. Previously, IntMap.findMin only returned the bound value, not the key. Data.Map returns the pair, so I changed IntMap to do the same. (This seems like common sense to me.)
Patch is attached. Can you tell I'm using this library a lot lately?
Scott
_______________________________________________ Libraries mailing list Libraries@haskell.org http://www.haskell.org/mailman/listinfo/libraries
On Thu, May 22, 2008 at 11:20:33AM -0700, Donald Bruce Stewart wrote:
Ross, are you able, as containers maintainer, to act as the designated reviewer of Scott's IntMap patches?
I'm confused; the latest Cabal file says maintainer: libraries@haskell.org as far as I can see. Are you saying that that is wrong? Or are you proposing that we change how the library submissions process (http://www.haskell.org/haskellwiki/Library_submissions) works? Thanks Ian
On Fri, May 23, 2008 at 03:43:47PM +0100, Ian Lynagh wrote:
On Thu, May 22, 2008 at 11:20:33AM -0700, Donald Bruce Stewart wrote:
Ross, are you able, as containers maintainer, to act as the designated reviewer of Scott's IntMap patches?
I'm confused; the latest Cabal file says maintainer: libraries@haskell.org as far as I can see. Are you saying that that is wrong? Or are you proposing that we change how the library submissions process (http://www.haskell.org/haskellwiki/Library_submissions) works?
That puzzled me too. But the library submissions process was only intended for interface changes, and Scott's changes to IntMap/IntSet construction and findMax are performance issues.
On Fri, May 23, 2008 at 8:54 AM, Ross Paterson
But the library submissions process was only intended for interface changes, and Scott's changes to IntMap/IntSet construction and findMax are performance issues.
Not entirely. I changed the signatures of IntMap.findMin and findMax to return the key/value pair. Previously they only returned the value. I believe this was a typo -- findMin was defined as "fst . runIdentity . minView" but it "should" have been "fst . runIdentity . minViewWithKey". I say "should" because Map.findMin also returns the pair, and the documentation for the functions made it seem like that's what they should return. But I don't think its a major compatibility issue because these functions are still not published in the Haddock indices ( http://www.haskell.org/ghc/docs/latest/html/libraries/containers/Data-IntMap...<-- no findMin there) so whoever is using them is doing so at the risk of code breakage. But if this still counts as API change I'd be glad to make another that keeps signatures unchanged. However I do think it is ridiculous for findMin to return only the value and not the key as well. Scott
ross:
On Fri, May 23, 2008 at 03:43:47PM +0100, Ian Lynagh wrote:
On Thu, May 22, 2008 at 11:20:33AM -0700, Donald Bruce Stewart wrote:
Ross, are you able, as containers maintainer, to act as the designated reviewer of Scott's IntMap patches?
I'm confused; the latest Cabal file says maintainer: libraries@haskell.org as far as I can see. Are you saying that that is wrong? Or are you proposing that we change how the library submissions process (http://www.haskell.org/haskellwiki/Library_submissions) works?
That puzzled me too.
But the library submissions process was only intended for interface changes, and Scott's changes to IntMap/IntSet construction and findMax are performance issues.
Yeah, these performance changes really need someone to sit down and test out the changes. They did a different kind of review to the usual API change, in my opinion. I act as this person for bytestring, but for containers, I'm not sure we have an obvious performance czar. If people are of the view that perf patches will receive the appropriate review -- or if it is Ian's job (as for say, the (^) issue in base), then that's good -- we can leave the process as is. -- Don
On Fri, May 23, 2008 at 08:49:25AM -0700, Donald Bruce Stewart wrote:
Yeah, these performance changes really need someone to sit down and test out the changes. They did a different kind of review to the usual API change, in my opinion.
Well, I thought that a big part of the reason for creating the library submissions process (LSP) was so that submitted patches didn't just get overlooked, with everyone assuming someone else would deal with it. If we don't want to use the LSP for performance patches, then we need something else, so that they don't get overlooked either. Thanks Ian
igloo:
On Fri, May 23, 2008 at 08:49:25AM -0700, Donald Bruce Stewart wrote:
Yeah, these performance changes really need someone to sit down and test out the changes. They did a different kind of review to the usual API change, in my opinion.
Well, I thought that a big part of the reason for creating the library submissions process (LSP) was so that submitted patches didn't just get overlooked, with everyone assuming someone else would deal with it.
If we don't want to use the LSP for performance patches, then we need something else, so that they don't get overlooked either.
So we have someone able to review performance related patches to containers? -- Don
participants (4)
-
Don Stewart -
Ian Lynagh -
Ross Paterson -
Scott Dillard