Added HasCallStack to partial functions #493
Conversation
|
You're going to need to wrap all those pieces in CPP restricting in to appropriate |
|
Is it a Thanks! |
|
Glasgow Haskell, because it really relies on GHC-specific features.
…On Jan 13, 2018 1:33 PM, "Daniel Winograd-Cort" ***@***.***> 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
<#493 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_UXmc9vmlNSnH23l5tJJgPHiyzIZks5tKPcXgaJpZM4RdSDv>
.
|
Data/Map/Internal.hs
Outdated
| updateAt :: (k -> a -> Maybe a) -> Int -> Map k a -> Map k a | ||
| #endif |
There was a problem hiding this comment.
Would it be better to just make this function total by making an update at a bad index do nothing?
There was a problem hiding this comment.
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?
Data/Map/Internal.hs
Outdated
| @@ -1588,7 +1607,11 @@ updateAt f !i t = | |||
| -- > deleteAt 2 (fromList [(5,"a"), (3,"b")]) Error: index out of range | |||
| -- > deleteAt (-1) (fromList [(5,"a"), (3,"b")]) Error: index out of range | |||
|
|
|||
| #if __GLASGOW_HASKELL__ >= 800 | |||
| deleteAt :: HasCallStack => Int -> Map k a -> Map k a | |||
| #else | |||
Data/Map/Internal.hs
Outdated
| @@ -2661,7 +2692,11 @@ mergeA | |||
| -- @only2@ are 'id' and @'const' 'empty'@, but for example @'map' f@, | |||
| -- @'filterWithKey' f@, or @'mapMaybeWithKey' f@ could be used for any @f@. | |||
|
|
|||
| #if __GLASGOW_HASKELL__ >= 800 | |||
| mergeWithKey :: (HasCallStack, Ord k) | |||
There was a problem hiding this comment.
Ehhhhh... Not sure we should bother with this one. mergeWithKey is for users who don't mind the risk of being shot in the foot.
Data/Sequence/Internal.hs
Outdated
| @@ -4300,7 +4348,11 @@ zipWith f s1 s2 = zipWith' f s1' s2' | |||
| s2' = take minLen s2 | |||
|
|
|||
| -- | A version of zipWith that assumes the sequences have the same length. | |||
| #if __GLASGOW_HASKELL__ >= 800 | |||
| zipWith' :: HasCallStack => (a -> b -> c) -> Seq a -> Seq b -> Seq c | |||
| #else | |||
There was a problem hiding this comment.
No no no. This is an internal function and is only used when we know it will succeed.
Data/Sequence/Internal.hs
Outdated
| @@ -4498,7 +4554,11 @@ draw (PQueue x ts0) = x : drawSubTrees ts0 | |||
| -- | 'popMin', given an ordering function, constructs a stateful action | |||
| -- which pops the smallest elements from a queue. This action will fail | |||
| -- on empty queues. | |||
| #if __GLASGOW_HASKELL__ >= 800 | |||
| popMin :: HasCallStack => (e -> e -> Ordering) -> State (PQueue e) e | |||
There was a problem hiding this comment.
No no no. This is an internal function.
|
You're right; separate PR. I guess I can ask the libraries list.
…On Jan 15, 2018 12:42 PM, "Daniel Winograd-Cort" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Data/Map/Internal.hs
<#493 (comment)>:
> 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
<#493 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_ZY7CVkokVE5QHC8ZpqGPor5xnfNks5tK44ggaJpZM4RdSDv>
.
|
| findMin :: IntSet -> Key | ||
| #endif | ||
| findMin Nil = error "findMin: empty set has no minimal element" |
There was a problem hiding this comment.
Does the call stack grow through the recursion here or elsewhere? If so, that's a big problem. You can check the Core to be sure. We know there are no Nils except at the root, but GHC does not! If the stacks build, you'll need to restructure the functions to fix that. Watch out for performance. If the times for the current benchmarks exercising these functions are too short to trust, consider adding more benchmarks.
There was a problem hiding this comment.
Wow, yes it does. I didn't realize it would do that. The fix is to use an internal go function. I'll update the PR with that in a moment.
On a related note, I realized that my HasCallStack for Map.! was wrong because Map.! calls find which actually throws the error. Is there a reason it uses find rather than lookup? My inclination is to remove find altogether (it's only used by ! and looks identical other than the Maybe wrapper) and replace with lookup---then ! can call error itself, which should then make HasCallStack work the way we want.
There was a problem hiding this comment.
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.
Data/Sequence/Internal.hs
Outdated
| @@ -4457,7 +4505,11 @@ unstableSortBy cmp (Seq xs) = | |||
| -- | fromList2, given a list and its length, constructs a completely | |||
| -- balanced Seq whose elements are that list using the replicateA | |||
| -- generalization. | |||
| #if __GLASGOW_HASKELL__ >= 800 | |||
| fromList2 :: HasCallStack => Int -> [a] -> Seq a | |||
There was a problem hiding this comment.
This is an internal function and shouldn't get a call stack. It's used to implement the IsList instance. Arguably, the method it's used to implement should get a call stack, but that method isn't really intended to be called directly in user code anyway, and GHC will always call it correctly.
|
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" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In Data/IntSet/Internal.hs
<#493 (comment)>:
> 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
<#493 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_ejufDBgEXQbVqoa6kUyuslx4sKpks5tK6LRgaJpZM4RdSDv>
.
|
Data/IntMap/Internal.hs
Outdated
| (!) m k = find k m | ||
| (!) m k | ||
| | Just a <- lookup k m = a | ||
| | otherwise = error ("IntMap.!: key " ++ show k ++ " is not an element of the map") |
There was a problem hiding this comment.
Using lookup will incur a Just allocation. One possible solution (for very recent GHC versions only) would be to define a lookup# function producing an unboxed sum:
type Maybe# a = (# Void# | a #)
pattern Nothing# :: Maybe# a
pattern Nothing# <- (# _ | #) where
Nothing# = (# void# | #)
pattern Just# :: a -> Maybe# a
pattern Just# a = (# | a #)
lookup# :: Key -> IntMap a -> Maybe# a
lookup# k m = ...
lookup k m = case lookup# k m of
Nothing# -> Nothing
Just# a -> Just aThe (potentially) great thing about this is that the Just gets applied on the "outside", so inlining and the case-of-case transformation will end up making it go away altogether:
m ! k = case
(case lookup# k m of
Nothing# -> Nothing
Just# a -> Just a) of
Nothing -> error ...
Just v -> v
-- ==> case-of-case, case of known constructor
m ! k =
case lookup# k m of
Nothing# -> error ...
Just# a -> aUnlike a CPS version, this theoretically helps user-written code as well. However, I have not actually tried benchmarking anything like this. And the CPP is potentially troublesome too.
There was a problem hiding this comment.
This seems cool -- it seems like it could be a fairly big change that would even prevent an allocation when one uses lookup (or related functions that return Maybe values) and immediately unwraps -- but it's a bit big for what I was hoping to contribute just now (also, a bit over my head). I think I'll take the easy way out and go back to using a find function to get a result without the extra allocation. Perhaps my next PR will be to overhaul the backend to use unboxed sums where possible :).
Data/Map/Strict/Internal.hs
Outdated
| @@ -887,9 +887,20 @@ atKeyIdentity k f t = Identity $ atKeyPlain Strict k (coerce f) t | |||
|
|
|||
| #if __GLASGOW_HASKELL__ >= 800 | |||
| updateAt :: HasCallStack => (k -> a -> Maybe a) -> Int -> Map k a -> Map k a | |||
| updateAt = go where | |||
There was a problem hiding this comment.
Does this actually give the right call stacks? Thinking about this more carefully, I suspect you want to check if the operation will succeed, and only then call a go function that takes no call stack. The alternative would be to call the go function using withFrozenCallStack, which I suspect will be more expensive. Benchmarks may tell.
There was a problem hiding this comment.
My tests show that, by default, functions like go do not have the HasCallStack constraint unless it is explicit in their type signatures. Therefore, this seems to give the right call stack. There's no need to check if the operation succeeds or not first, and because go doesn't have the constraint, there's no need for withFrozenCallStack.
|
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" ***@***.***> 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
<#493 (comment)>,
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 ( |
|
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 |
|
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 ***@***.***> 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
<#493 (comment)>,
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 |
|
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 ***@***.***> 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
<#493 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABzi_dz5M8A6kX9EeWay0qg1O9RFAXR0ks5u8VGHgaJpZM4RdSDv>
.
|
As discussed on Issue #489, this PR adds
HasCallStackto 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.