Fix insert worker/wrapper issues

[treeowl]

The new pointer equality version of insert in Data.Map led to
a severe regression in the last-piece benchmark of nofib.
It turned out that worker/wrapper was doing absolutely horrible
things to insert, breaking the pointer equality tests and
also leading to completely unnecessary allocation. This commit
adds horrible hacks that seem to prevent this from happening.

[ezyang]

@treeowl, is the unboxing that GHC does here ever profitable? (As in, does it improve anything at all? I haven't looked at the core but it doesn't look there is anything that actually makes use of the unboxed version. Are you talking about the specialized version, maybe?

If the unboxing is always unprofitable, it's worth filing a GHC bug with a small test case requesting that GHC become smart enough not to unbox if it can't profitably use the unboxed value.

[treeowl]

I believe the unboxed version is used for the comparisons to find the insertion point. I certainly hope so. We really want to have both boxed and unboxed here. On Feb 23, 2017 4:46 AM, "Edward Z. Yang" <notifications@github.com> wrote: @treeowl <https://github.com/treeowl>, is the unboxing that GHC does here ever profitable? (As in, does it improve anything at all? I haven't looked at the core but it doesn't look there is anything that actually makes use of the unboxed version. Are you talking about the specialized version, maybe? If the unboxing is always unprofitable, it's worth filing a GHC bug with a small test case requesting that GHC become smart enough not to unbox if it can't profitably use the unboxed value. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#416 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_YDAonLnY6CTL9NvdmI8UwBddARUks5rfVV8gaJpZM4MJFYW> .

[winterland1989]

These pointer equality hacks looks horrible to me, what's the motivation for this "better" union anyway?

[treeowl]

We want to share as much structure as possible between the result and the arguments. The pointer equality tricks allow us to avoid allocating new structure that's exactly the same as structure we already have.

…

On Mar 8, 2017 10:00 PM, "winterland" ***@***.***> wrote: These pointer equality hacks looks horrible to me, what's the motivation for this "better" union anyway? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#416 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_YfLOioc_dVdqcQax0Bv4NdqH2Phks5rj2tSgaJpZM4MJFYW> .

[winterland1989]

I mean under what circumstances we would insert sharing values into a Map? From my understanding immutable data structures doesn't work that way. This kind of optimizations never reach a end: elem , nub... because I don't think there is an use case. Correct me if you can find one .

[treeowl]

I have no idea what you're getting at.

…

On Mar 8, 2017 10:40 PM, "winterland" ***@***.***> wrote: I mean under what circumstances we would insert sharing values into a Map? From my understanding immutable data structures doesn't work that way. This kind of optimizations never reach a end: elem , nub... because I don't think there is an use case. Correct me if you can find one . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#416 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_cCmgE4FSVQ93YnL_bP-bYQ6bvf8ks5rj3SngaJpZM4MJFYW> .

[winterland1989]

OK, let me try to put it in another way: under what practical circumstances l' `ptrEq` l would happen?

[wrengr]

@winterland1989 the situation where pointer equality would be true is when the inserted value is pointer-equal to the value that's already there. For insert, I agree that this seems unlikely to occur often under practical circumstances. (Though it @treeowl has some examples, that'd be great.) Conversely, for functions like filter, it's very easy to imagine cases where the filter passes everything in some subtree.

[winterland1989]

I still have a hard time understand why we introduce this hack. @treeowl , can you show me some use cases of the pointer equality version insert/merge except some benchmark code?

[winterland1989]

And here is recently discussion on reallyUnsafePointerEquality: https://mail.haskell.org/pipermail/haskell-cafe/2010-June/079532.html

From the discussion above i also doubt current code's safety: if reallyUnsafePointerEquality give false positive result, insert/merge will be incorrect.

[treeowl]

It can't give a false positive; it can only give a false negative.

…

On Nov 23, 2017 1:27 AM, "winterland" ***@***.***> wrote: And here is recently discussion on reallyUnsafePointerEquality: https://mail.haskell.org/pipermail/haskell-cafe/2010-June/079532.html From the discussion above i also doubt current code's safety: if reallyUnsafePointerEquality give false positive result, insert/merge will be incorrect. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#416 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_Ufsps_PowBKkUa6fjEpsgVGLZXpks5s5RA7gaJpZM4MJFYW> .

[winterland1989]

The link above is mentioned in recently discussion here(Obviously i posted the wrong link):
https://mail.haskell.org/pipermail/haskell-cafe/2017-November/128208.html

What i want is a demonstration of why we're doing this. AFAICT if you're facing sharing detection problems in pure functional data structures, you're probably doing it wrong.

[treeowl]

The reason to want this is to preserve sharing when possible. I don't understand this debate about false positives. How can GC *ever* happen while such a primop is running? It's not like it allocates anything. If that's a problem, then it's an RTS bug anyway, and not my problem.

…

On Nov 23, 2017 4:37 AM, "winterland" ***@***.***> wrote: The link above is mentioned in recently discussion here(Obviously i posted the wrong link): https://mail.haskell.org/pipermail/haskell-cafe/2017-November/128208.html What i want is a demonstration of why we're doing this. AFAICT if you're facing sharing detection problems in pure functional data structures, you must do it wrong. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#416 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_fcT_SId3Z0ptdQMdiRzDzbBumKNks5s5TzbgaJpZM4MJFYW> .

[winterland1989]

I can get what you want to solve from this comment:

    -- Unlike insertR, we only get sharing here
    -- when the inserted value is at the same address
    -- as the present value. We try anyway; this condition
    -- seems particularly likely to occur in 'union'.

But the question is, when we will insert a value with a same address? Isn't there something wrong with the algorithm design? Can you provide some use case?

[AndreasPK]

But the question is, when we will insert a value with a same address? Isn't there something wrong with the algorithm design? Can you provide some use case?

Combining any two maps which have some shared values.

let usedStuff = usedByA `union` usedByB

I know I have written code like that in the past.

[winterland1989]

But the question is, when we will insert a value with a same address? Isn't there something wrong with the algorithm design? Can you provide some use case?

Combining any two maps which have some shared values.

let usedStuff = usedByA `union` usedByB

I know I have written code like that in the past.

I would be interested if you can provide some code samples (Input and output included). I'm pretty sure there is a way to rewrite the algorithm to eliminate merging sharing values because you can statically know what is shared between maps under immutable settings.

[AndreasPK]

I can't remember the exact project so sadly I can't give the code.

And yes you can always rewrite the code to avoid duplicating inserts.
But there really are only two ways to know if two distinct maps share data.

• You store that information somewhere else (more heap useage).
• You compute that information when you need it (which the ugly pointer hack does very efficiently).

Maybe that work is still more overhead than what we gain. Maybe not.

But if you feel this should be removed the way forward is to open an issue and provide benchmarks of improvements for the general case.

[winterland1989]

And yes you can always rewrite the code to avoid duplicating inserts.
But there really are only two ways to know if two distinct maps share data.

No, we can statically know if two distinct maps share data since inserted data is never changed. e.g. we insert the same binding into two different maps.

But if you feel this should be removed the way forward is to open an issue and provide benchmarks of improvements for the general case.

The problem is that the patch introduced a special benchmark (above transformation also apply) to benchmark this bogus case.

[treeowl]

No, you can't statically know if two maps share data, in general. For example, one map might consist of information obtained from one source and the other may consist of information obtained from another source. When the maps are combined, some information may have been obtained from both sources. @AndreasPK meant that you should come up with benchmarks showing that the pointer equality checks cause significant harm in typical cases. Since the checks are extremely cheap, I think that will be difficult to do, but feel free to try to prove me wrong.

[winterland1989]

Your argument also applies to all theEq instances, e.g. when comparing inner fields. We generally don't do pointer equality because under immutable settings we can generally factor out the information you mentioned. This is also why I asked for a practical example of combining two maps containing shared values except for some benchmark snippets. Pointer equality is indeed cheap to check, but I hardly can imagine a situation when I need them except heap objects with identities, such as MutableByteArray#s or MutVar# s.

[AndreasPK]

No, we can statically know if two distinct maps share data since inserted data is never changed. e.g. we insert the same binding into two different maps.

Please explain how can you "statically" know that two maps, constructed from two different dynamically generated heap objects, contain the same data. Clearly both me and @treeowl think differently so it's on you to proof us wrong.

The problem is that the patch introduced a special benchmark (above transformation also apply) to benchmark this bogus case.

How is that a problem? Just remove the pointer checks, remove that special case and report the change in performance. If it's significant then a discussion about removing it makes sense, otherwise not.

This is also why I asked for a practical example of combining two maps containing shared values except for some benchmark snippets. Pointer equality is indeed cheap to check, but I hardly can imagine a situation when I need them except heap objects with identities, such as MutableByteArray#s or MutVar# s

I can give you pseudo code for a Set example:

data Expr = 
    Let Id Expr Expr | ... | Var Id

usedIds :: Ast -> S.Set Id
usedIds (Let id expr body) = 
    S.union (usedIds expr) (usedIds body)
usedIds (Var id) = S.singleton id
usedIds ...

Id's all originate from binds, so they all reference the same heap object. So the pointer equality will hold.

This is not a contrived example GHC contains code like this, with the only difference being it uses IntSet.

[winterland1989]

Your example is convincing so I searched for the old discussion try to remember why I have a problem with pointer equality in general, so here it's (an old discussion started by myself): https://www.reddit.com/r/haskell/comments/4ivvge/why_no_reference_equality/

To sum it up, here're problems with built-in pointer equality as far as I can see:

• It won't work reliably under optimization and GC settings.
• It doesn't work for some types, such as NaN /= NaN :: Double. Thus lead to different results (actually we should never assume pointer equality can give the same result with Eq instances).
• We can always using customized identity field to get the same performance, e.g. If Id type is a large product in your example, then we can add our own identity field to speed up the comparison.

So I'm still in a position objecting adding implicit pointer equality by default, but maintainers' views differ. Out of curiosity, If those Id s point to same heap objects, how are they generated?

[AndreasPK]

It won't work reliably under optimization and GC settings.

It works reliable under specific circumstances. As Augustsson explained:

It is possible to use pointer equality to speed up regular equality in Haskell, but it's rather tricky since you want to maintain the same semantics. E.g., if you know that two objects are fully evaluated and cycle free (i.e., regular equality will not loop), then you can use pointer equality as a fast first test for equality.

It doesn't work for some types, such as NaN /= NaN :: Double. Thus lead to different results (actually we should never assume pointer equality can give the same result with Eq instances).

But it does (for this use case)!
Assume you have insert (pt1@NaN, pt2@Foo) into the map, and the map already contains (pt1@NaN,pt2@Foo).

• Without pointer equality they compare unequal, we overwrite the existing entry with (pt1,Foo). All the keys/elements in the map are the same.
• With pointer equality they compare equal, we don't overwrite the existing entry since the existing Key and Value are already in the map. All the keys/elements in the map are the same.
• If the pointers don't match it's a moot point.

So I'm still in a position objecting adding implicit pointer equality by default

That is fair. We can hate the hack, but it's correct and if it gives better performance then it is a good fit for such a essential package as containers. I also would never use ptr Equality by default.

We can always using customized identity field to get the same performance, e.g. If Id type is a large product in your example, then we can add our own identity field to speed up the comparison.

We can. It's also what GHC does. But if we can better performance for this case without significantly impacting the general case performance why wouldn't we.

Out of curiosity, If those Id s point to same heap objects, how are they generated?

Map lookup.

I will leave this discussion now. Getting rid of pointer equality because it's hacky won't happen. High performance code tends to be hacky. But if you think it makes containers performance worse and can show that then I don't think there will be a lot of resistance.

But if you can do so then open a ticket for it. Discussing it on a closed merge request is kinda pointless.

[winterland1989]

Fair enough, I will try make a benchmark for this some day.