haskell
Added HasCallStack to partial functions
As discussed on Issue https://github.com/haskell/containers/issues/489, this PR adds HasCallStack to the constraints of partial functions. I think I got all of the relevant partial functions, but I'd be happy for someone to double check that I didn't miss anything.
I ran the benchmarks (stack bench) before and after these changes and have attached them (before_hascallstack.bench.txt, after_hascallstack.bench.txt). I scanned through the results, trying to pay attention to tests on partial functions specifically, but I didn't notice any changes (outside of expected deviations within the margin of error).
Please let me know if there's more that I can do to help get this PR successfully merged.
You're going to need to wrap all those pieces in CPP restricting in to appropriate __GLASGOW_HASKELL__ versions.
Is it a __GLASGOW_HASKELL__ version or is it MIN_VERSION_base(4,9,0)?
Thanks!
Glasgow Haskell, because it really relies on GHC-specific features.
On Jan 13, 2018 1:33 PM, "Daniel Winograd-Cort" [email protected] wrote:
Is it a GLASGOW_HASKELL version or is it MIN_VERSION_base(4,9,0)?
Thanks!
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/containers/pull/493#issuecomment-357456333, or mute the thread https://github.com/notifications/unsubscribe-auth/ABzi_UXmc9vmlNSnH23l5tJJgPHiyzIZks5tKPcXgaJpZM4RdSDv .
You're right; separate PR. I guess I can ask the libraries list.
On Jan 15, 2018 12:42 PM, "Daniel Winograd-Cort" [email protected] wrote:
@dwincort commented on this pull request.
In Data/Map/Internal.hs https://github.com/haskell/containers/pull/493#discussion_r161581989:
updateAt :: (k -> a -> Maybe a) -> Int -> Map k a -> Map k a +#endif
This is definitely a more interesting question than just where to add HasCallStack and might make more sense for a different issue/PR. Personally, I'm not sure I've ever used the ***At functions, so I'm not really the target audience to be weighing in on this decision. Is there a way you could get community feedback about such a change? Maybe Haskell-cafe?
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/containers/pull/493#discussion_r161581989, or mute the thread https://github.com/notifications/unsubscribe-auth/ABzi_ZY7CVkokVE5QHC8ZpqGPor5xnfNks5tK44ggaJpZM4RdSDv .
You may be able to avoid capturing issues by giving the go functions full
signatures, making it clear they don't take call stacks. Check Core often,
maybe look at STG occasionally, and benchmark.
On Jan 15, 2018 2:11 PM, "Daniel Winograd-Cort" [email protected] wrote:
@dwincort commented on this pull request.
In Data/IntSet/Internal.hs https://github.com/haskell/containers/pull/493#discussion_r161596845:
findMin :: IntSet -> Key +#endif findMin Nil = error "findMin: empty set has no minimal element"
Oh, I just noticed the [Note: Local 'go' functions and capturing] -- I'll read that and try to make sure I'm not doing anything stupid.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/containers/pull/493#discussion_r161596845, or mute the thread https://github.com/notifications/unsubscribe-auth/ABzi_ejufDBgEXQbVqoa6kUyuslx4sKpks5tK6LRgaJpZM4RdSDv .
Are you making any progress? I'd love to get this into the next release, but that's probably going to happen in the morning!
I'm sorry, but I got caught up in other work this past week and haven't worked on this at all. I guess it will have to wait until the next release :/.
That's fine. Just do the best you can.
On Jan 22, 2018 12:03 PM, "Daniel Winograd-Cort" [email protected] wrote:
I'm sorry, but I got caught up in other work this past week and haven't worked on this at all. I guess it will have to wait until the next release :/.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/containers/pull/493#issuecomment-359493298, or mute the thread https://github.com/notifications/unsubscribe-auth/ABzi_c75_mZ3BnpLEp2e55_OwTuJm3S4ks5tNL9ugaJpZM4RdSDv .
More generally, I must confess that I really don't know how to check Core. I think I know how to look at Core (-ddump-simpl?), but I don't know what to do with it or how to tell if it's good or bad.
-ddump-simpl indeed. I suggest that for a while you should add -dsuppress-all -dno-suppress-type-signatures. You want to look for fishy things like recursively growing call stacks.
Separately, you should test each function with bad arguments of all relevant sorts and make sure the call stack indicates that the problem is with the bad code that called the partial function, not the partial function itself.
What is the status on this? I wound up writing custom wrappers around some of the functions in Data.Map for stack traces, but it would be awesome if this were upstreamed.
No one has done the grunt work to check on whether this is optimized the way we want and whether the performance regressions are tolerable.
On Mon, Dec 24, 2018 at 3:52 PM Siddharth [email protected] wrote:
What is the status on this? I wound up writing custom wrappers around some of the functions in Data.Map for stack traces, but it would be awesome if this were upstreamed.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/containers/pull/493#issuecomment-449770506, or mute the thread https://github.com/notifications/unsubscribe-auth/ABzi_WC-HcAgRNGMV6WTKUp-wxfYMOGJks5u8T55gaJpZM4RdSDv .
Is it necessary to pay a performance cost? Can we not expose a Data.Map.{Strict, Lazy}.Debug that contains the functions that expose debug info?
Regardless, the way we proceed will depend on the performance cost. If it's
low enough, I think it's okay to force users to accept it. Otherwise not.
But either way, we want to be sure that the cost is as low as possible. In
particular, we definitely want to make sure that we never build up call
stacks recursively. The call stacks are for debugging code that uses the
package, and explicitly not for debugging containers itself.
On Mon, Dec 24, 2018 at 5:13 PM Siddharth [email protected] wrote:
Is it necessary to pay a performance cost? Can we not expose a Data.Map.{Strict, Lazy}.Debug that contains the functions that expose debug info?/
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/containers/pull/493#issuecomment-449774338, or mute the thread https://github.com/notifications/unsubscribe-auth/ABzi_dz5M8A6kX9EeWay0qg1O9RFAXR0ks5u8VGHgaJpZM4RdSDv .