Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider depending on primitive #197

Open
treeowl opened this issue Mar 21, 2018 · 13 comments · May be fixed by #206
Open

Consider depending on primitive #197

treeowl opened this issue Mar 21, 2018 · 13 comments · May be fixed by #206

Comments

@treeowl
Copy link
Collaborator

treeowl commented Mar 21, 2018

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
Copy link
Collaborator

tibbe commented Mar 21, 2018 via email

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 21, 2018

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
Copy link
Collaborator

tibbe commented Mar 21, 2018 via email

@andrewthad
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Collaborator

tibbe commented Apr 20, 2018

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
Copy link
Contributor

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
Copy link
Collaborator Author

treeowl commented Apr 20, 2018 via email

@andrewthad
Copy link
Contributor

I'm working on depending on primitive on #206

@sjakobi
Copy link
Member

sjakobi commented Mar 20, 2022

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
Copy link
Collaborator Author

treeowl commented Mar 21, 2022

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
Copy link
Member

sjakobi commented Mar 21, 2022

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

@treeowl
Copy link
Collaborator Author

treeowl commented Mar 21, 2022

The tricky thing with primitive is inlining decisions. It's critical to specialize to the right MonadPrim (or whatever it's called) instance, and to do it early. But there's only so much control a library has over that. Building primitive on a solid, monomorphic foundation of ST, and exposing that foundation, means that when working with something that is definitely ST s (which is really the fundamental instance) you don't have to worry about specialization. It's trivial to also expose a monomorphic version for IO ~= ST RealWorld which just coerces the ST versions. The "classy" versions can be built on top quite mechanically (with a few exceptions, I think).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants