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

Why aren't constructors exported out of Internal libraries #342

Closed
madgen opened this issue Dec 21, 2021 · 6 comments · Fixed by #347
Closed

Why aren't constructors exported out of Internal libraries #342

madgen opened this issue Dec 21, 2021 · 6 comments · Fixed by #347

Comments

@madgen
Copy link

madgen commented Dec 21, 2021

Hi, I'm trying to derive Lift instances for HashMap and HashSet as done in th-lift-instances for Set and Map from containers (specifically here) so that I can use them with Typed Template Haskell.

However, deriving Lift as an orphan instance requires access to the constructors which I'd expect from the Internal version of the HashSet and HashMap modules but that is not the case. HashMap partially exposes it, however, it uses types such as HashMap.Array.Array which doesn't expose its constructors.

Is there a reason for this?

@treeowl
Copy link
Collaborator

treeowl commented Dec 21, 2021

We should fix this. We should also offer a Lift instance. That said, I'm rather curious how you came to need that instance.

@madgen
Copy link
Author

madgen commented Dec 21, 2021

@treeowl I'm staging a regular expression compiler. I switched to HashMap/HashSet for efficiency, but lost the ability to stage without doing horrible hacks. The following works if I monomorphise all my type signatures for example:

instance (Lift k, Lift v) => Lift (M.HashMap k v) where
  lift m = [| M.fromList m' |]
    where
    m' = M.toList m
  liftTyped = unsafeTExpCoerce . lift

I'm hoping if I instead just use DeriveLift, I can keep being polymorphic in my signatures and avoid cryptic TH errors.

@treeowl
Copy link
Collaborator

treeowl commented Dec 21, 2021

We won't be able to "just" derive Lift, because we'll have to explain how to lift the arrays. Not a big deal; any code we're missing can be copied from primitive. Your orphan workaround should probably be written

instance (Lift k, Lift v) => Lift (M.HashMap k v) where
  liftTyped m = [|| M.fromList m' ||]
    where
      m' = M.toList m

to avoid the ugly and unnecessary unsafety.

treeowl added a commit to treeowl/unordered-containers that referenced this issue Dec 21, 2021
treeowl added a commit to treeowl/unordered-containers that referenced this issue Dec 21, 2021
@treeowl
Copy link
Collaborator

treeowl commented Dec 21, 2021

@madgen I've opened a PR. As it is right now, the internal arrays are realized as lists and converted. I imagine that will lead to more compact code than trying to produce code to actually construct the arrays by poking elements into arrays one by one. However, I'd be happy to experiment with alternative approaches if you find it's taking too long to build the maps at run-time.

@phadej
Copy link
Contributor

phadej commented Dec 21, 2021

As I commented in the PR #343, it might well be that using containers with DeriveLifted instance will be faster then HashMap which cannot be lifted into static structure.

See https://well-typed.com/blog/2020/06/th-for-static-data/

Please benchmark whether HashMap would be faster, I'm be curious to know! GHC devs would appreciate a real world motivated push for https://gitlab.haskell.org/ghc/ghc/-/issues/16944

@madgen
Copy link
Author

madgen commented Dec 21, 2021

@phadej compared to the DeriveLift + containers from th-lift-instances, my stupid hack with HashMap Lift instance + monomorphisation of the staged function type signatures led to a 5x improvement in my microbenchmarks of the staged code. However, to be sure that monomorphisation of signatures didn't have any effect, I should try the containers variant with monomorphisation as well.

Unfortunately even with monomorphisation, I couldn't get HashSet to be staged, but it is WIP. I can update you once (if) I get that working too.

Note that the performance of unstaged versions of both container and unordered-container implementations are nowhere near the staged ones. There's a 100x perf difference.

I'll try benchmarking with the Lift instances you provided as well. Thank you.

treeowl added a commit to treeowl/unordered-containers that referenced this issue Dec 22, 2021
treeowl added a commit to treeowl/unordered-containers that referenced this issue Dec 22, 2021
treeowl added a commit to treeowl/unordered-containers that referenced this issue Dec 22, 2021
sjakobi added a commit that referenced this issue Jan 6, 2022
sjakobi added a commit that referenced this issue Jan 11, 2022
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