Consider depending on primitive

[treeowl]

We try hard to limit dependencies, but it seems unfortunate that Data.HashMap.Array and Data.Primitive.Array are so very similar. How bad would it be to use it? There are a few serious bugs in the released Data.Primitive.Array right now, but fixes have been merged and a test suite is on its way.

[tibbe]

I tried to use it initially and it didn't work out well. I believe there was some issues with inlining.

…

On Wed, Mar 21, 2018, 11:34 David Feuer ***@***.***> wrote: We try hard to limit dependencies, but it seems unfortunate that Data.HashMap.Array and Data.Primitive.Array are so very similar. How bad would it be to use it? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#197>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDfcmhI-LUUf5trwwot5OpmkK6dPMaks5tgdhGgaJpZM4SzAHC> .

[treeowl]

Hrmf. Well, if it won't work it won't work, but it might be worth seeing if it can be brought up to snuff. Speaking of inlining, I'm wondering what, if anything, that is currently marked INLINE should actually be marked INLINABLE. At the moment, any code using HashMap heavily is likely to end up very large.

[tibbe]

Inlinable is very tricky. For example, a function marked inline calling a function marked inlinable will make specialization fail due to optimization pass ordering. I filled a GHC bug about it long time ago.

…

On Wed, Mar 21, 2018, 11:39 David Feuer ***@***.***> wrote: Hrmf. Well, if it won't work it won't work, but it might be worth seeing if it can be brought up to snuff. Speaking of inlining, I'm wondering what, if anything, that is currently marked INLINE should actually be marked INLINABLE. At the moment, any code using HashMap heavily is likely to end up very large. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#197 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDfc3fIDuScz5fdFWf0oFk1eCXR3OIks5tgdmagaJpZM4SzAHC> .

[andrewthad]

At some point after the next version of primitive is released, I'd like to give depending on primitive a try. At this point, nearly everything in primitive is explicitly given an INLINE pragma. I don't know how long ago the original attempt at doing this was, but maybe things in primitive have changed in the meantime. In particular, I cannot see any reason why the functions in Data.HashMap.Array would inline any better than their equivalents in Data.Primitive.SmallArray.

@tibbe Let's say that I am able to depend on the array implementation from primitive, and the benchmark suite shows no degradation in performance. Are there other reasons for which this change would be unwanted?

[andrewthad]

By "other reasons for which this change would be unwanted", what I'm mostly asking is whether there's an objection to depending on primitive at all. Such a dependency would lessen control over some of the source code and would make it possible for regressions in primitive to affect unordered-containers. I just want to make sure that if I have a go at this, that there aren't other factors outside of the aforementioned inlining failures that are preventing the primitive dep from happening.

[tibbe]

Personally I think primitive is fine if it doesn't hurt performance but I'll leave it up to the primary maintainer to decide.

[andrewthad]

The only maintainer listed on hackage is you. I was not aware that there was another, but looking through the commit history makes me suspect it's David. If so, we should update this information in the cabal file.

[treeowl]

Johan, if I create a haskell-unordered-containers github organization, would you transfer the repository there? Organizations make it a lot easier to deal with maintainers coming and going.

…

On Fri, Apr 20, 2018 at 11:23 AM, Andrew Martin ***@***.***> wrote: The only maintainer listed on hackage is you. I was not aware that there was another, but looking through the commit history makes me suspect it's David. If so, we should update this information in the cabal file. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#197 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_TAyjGKP-8pgowpyhFOMkvY1o_VQks5tqf1ogaJpZM4SzAHC> .

[andrewthad]

I'm working on depending on primitive on #206

[sjakobi]

I used to be undecided about this idea, but I'm pretty positive now:

• We get rid of a bunch of tricky code with a weird naming scheme.
• Instead we'll use code that has more users and is therefore more likely to be performant and correct.
• If we run into any SmallArray-related issues, we can potentially get help from the primitive maintainers and other users.

@andrewthad Could you rebase your PR #206?

Regarding the risk of subtle performance regressions: I plan on reviewing the Core and STG diffs for #206 and I'm fairly optimistic that this will catch most issues.

[treeowl]

I'll give this a "yes, but". I actually think the Right Thing is to start by modifying primitive to implement the operations in ST (in separate modules) and then lift them into the primitive classes. Then there won't be regressions here or elsewhere. I did this for the experimental and I think currently broken primitive-unlifted package, and it was quite nice all around.

[sjakobi]

@treeowl Can you clarify why this approach is better? It might also be helpful to link to the work you mention.