Add Lift instances #343

treeowl · 2021-12-21T02:41:25Z

Starts work on #342

unordered-containers.cabal

phadej · 2021-12-21T09:11:15Z

Data/HashMap/Internal/Array.hs

+
+instance TH.Lift a => TH.Lift (Array a) where
+#if MIN_VERSION_template_haskell(2,16,0)
+  liftTyped ar = [|| fromList' arlen arlist ||]


It's disappointing that SmallArray# (and other *Array#) don't have literals, but I don't think we can do better atm.

Fortunately, I don't that any overhead converting the arrays from lists will really be noticeable, especially since I expect all this spliced stuff gets floated out as CAFs. Literals would be nice.

they will be CAFs, but they could been off-heap structures, which would been even better.

See https://well-typed.com/blog/2020/06/th-for-static-data/#disclaimer-this-wont-work-for-all-data-types

You mean just in the program code? Do we have things with pointers that work like that?

E.g. Map and IntSet, yes.

sjakobi

To fully fix #342, we should also expose the Array constructor in Data.HashMap.Internal.Array, and for better ergonomics also expose the HashMap constructors in Data.HashMap.Internal.Strict.

unordered-containers.cabal

sjakobi · 2021-12-21T14:01:25Z

Data/HashMap/Internal/Array.hs

+fromList' :: Int -> [a] -> Array a
+fromList' n xs0 =
+    CHECK_EQ("fromList'", n, Prelude.length xs0)
+        run $ do
+            mary <- new_ n
+            go xs0 mary 0
+  where
+    go [] !mary !_   = return mary
+    go (!x:xs) mary i = do write mary i x
+                           go xs mary (i+1)


What's the motivation for adding this in addition to the existing fromList which looks identical at first glance?

It's strict in the elements, so the resulting HashMap won't be full of thunks.

Ah! Can you add haddocks that point this out?

Ah, but the leaves may still be thunks .... Hmm.... Maybe it's better to be lazy after all?

Maybe it's better to be lazy after all?

@madgen, what do you think / prefer?

@sjakobi in my particular application laziness doesn't play much role, so I prefer it there weren't full of thunks, but I don't think that is a superior choice in general. Do what you think would be most generally applicable please.

@sjakobi, The problem is that it's not a "full of thunks" vs. "not full of thunks". If we're lazy in the leaves, as we really should be, then we should be lazy in the spines as well for best results. But users may want to be strict in the leaves, so we should have a strict lift in the Strict module.

I don't understand. What you mean by leaves. Elements in the HashMap? Isn't the assumption that they will be floated out and made static data if they can?

(That's why I'd like SmallArray# literals, you'll just know that lifted expressions won't have any thunks as it perfectly would be just constructors or literals all the way).

@phadej, I guess we want to force construction of the values? Will the derived instance do that? I got a bit confused again.

treeowl · 2021-12-21T14:13:24Z

To fully fix #342, we should also expose the Array constructor in Data.HashMap.Internal.Array, and for better ergonomics also expose the HashMap constructors in Data.HashMap.Internal.Strict.

You're right; this isn't a full fix.

treeowl · 2021-12-22T21:13:25Z

@phadej, and @madgen, could you confirm that my commit building the values eagerly is the reasonable thing? If so, I'll squash this all and merge. We should open a ticket to use SmallArray literals if those ever become available.

Closes haskell-unordered-containers#342

Co-authored-by: Simon Jakobi <[email protected]>

treeowl · 2021-12-22T23:30:22Z

Note: StandaloneDeriving is needed for the @since annotations under GHC 8.0.2. Those weren't yet allowed in deriving lists.

madgen · 2021-12-23T00:28:54Z

@phadej, and @madgen, could you confirm that my commit building the values eagerly is the reasonable thing? If so, I'll squash this all and merge. We should open a ticket to use SmallArray literals if those ever become available.

I'll test it against my code tomorrow and let you know. Sorry, I just couldn't get my head off of work in the past two days.

madgen · 2021-12-26T12:08:25Z

@treeowl I tried it on my project. It works like a charm I still see 100x perf benefit.

However, as with my original hacky implementation, I can only get it work using monomorphisation. I think this diff still moves the needle in the right direction. So I suggest we go with it now.

I'll produce a minimal repro and inquire about why we are losing expressivity and maybe some TTH gurus can suggest a better way.

madgen · 2021-12-26T12:50:40Z

Also I checked the performance against containers, both the staged and unstaged versions are ~4x faster for unordered-containers than their containers counterparts.

treeowl · 2021-12-26T14:05:35Z

Thanks for checking, @madgen. Could you explain what you mean about monomorphisation?

madgen · 2021-12-26T17:03:32Z

@treeowl

The function I want to stage has the type and it uses both HashSet and HashMap.

recognise :: forall a. (Eq a, Hashable a) => NFA a -> String -> Bool

So I'd love to stage it as

sRecognise :: forall a. (Eq a, Hashable a, Lift a) => NFA a -> Q (TExp (String -> Bool))

However, if I do this, then I get a type error that says a in Eq a is ambiguous and the instance cannot be resolved because of the call to HashMap.lookup. The error does not happen where sRecognise is defined but it happens where it is used and expanded by GHC.

This problem does not happen with the Lift instances for containers. That is why I hoped deriving Lift rather than handwriting the instances (at least partially) might resolve the problem.

The only way around I could find was to instead give the staged function the following signature where a is specialised to Int. This shouldn't be necessary and it is not ideal, but it works for now.

sRecognise :: NFA Int -> Q (TExp (String -> Bool))

Here's the implementation for completeness:

sRecognise :: forall a. (Eq a, Hashable a, Lift a) => NFA a -> Q (TExp (String -> Bool))
sRecognise NFA{..} = [|| \str ->
  case foldl $$(step) (Just _startState) str of
    Just st -> S.member st _acceptingStates
    Nothing -> False
  ||]
  where
  trFx = transitionFunction _moves

  step :: Q (TExp (Maybe a -> Char -> Maybe a))
  step =
    [||
      \mSt c -> do
        st <- mSt
        (st, c) `M.lookup` trFx
    ||]

treeowl · 2021-12-26T17:13:10Z

I'm guessing you might want ExtendedDefaultRules?

phadej · 2021-12-26T17:13:44Z

That is an old (T)TH problem. The types are not preserved, and the spliced AST is re-typechecked. Sometimes you get ambiguity errors. Very easy to get them with integral types, as literals like 5 are easily ambiguous.

You can workaround that by using https://hackage.haskell.org/package/lift-type-0.1.0.1/docs/LiftType.html, though it doesn't add a helpful utility:

annotate :: Typeable a => TExpQ a -> TExpQ a
annotate ...

but you can write it with a bit of unsafeTExpCoerce.

https://mpickering.github.io/papers/staged-sop.pdf desribes a problem in more detail (5.3 Lifting Type Variables)
https://xnning.github.io/papers/staging-with-class.pdf proposes a solution (it's about lifting type-classes,, you'd want to say CodeC (Eq a), CodeC (Hashable a), not Eq a, Hashable a).

madgen · 2021-12-26T17:38:15Z

@treeowl ExtendedDefaultRules did the trick! Thank you.

@phadej, do you know why the containers instances provided in th-lift-instances doesn't suffer from the same problem?

Thanks for the reading material. I'll get on that now.

phadej · 2021-12-26T18:49:44Z

@madgen, hard to say. -ddump-splices and look how they are different.

treeowl · 2021-12-26T19:27:29Z

@madgen , Hashable isn't a standard class specified by the Report, so variables with a Hashable constraint won't be defaulted without ExtendedDefaultRules.

madgen · 2021-12-26T19:30:06Z

@phadej @treeowl Both of you have been awesome. Thanks for all the help and for the Lift instances. Let me know if I can return the favour.

treeowl force-pushed the lift branch from 201b476 to 352ab8f Compare December 21, 2021 02:50

treeowl self-assigned this Dec 21, 2021

treeowl requested review from phadej and sjakobi and removed request for phadej December 21, 2021 02:55

phadej approved these changes Dec 21, 2021

View reviewed changes

sjakobi reviewed Dec 21, 2021

View reviewed changes

phadej mentioned this pull request Dec 21, 2021

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

Closed

treeowl force-pushed the lift branch 4 times, most recently from 76f9f33 to 8ae3071 Compare December 22, 2021 23:25

treeowl and others added 3 commits December 22, 2021 18:28

Add Lift instances

0e53a1d

Closes haskell-unordered-containers#342

Update unordered-containers.cabal

e4a8963

Co-authored-by: Simon Jakobi <[email protected]>

Build values eagerly on lift

8d9679b

treeowl force-pushed the lift branch from 8ae3071 to 8d9679b Compare December 22, 2021 23:29

treeowl mentioned this pull request Dec 23, 2021

Use array literals and/or static arrays if and when they become available #346

Open

treeowl merged commit 6910660 into haskell-unordered-containers:master Dec 26, 2021

Bodigrim mentioned this pull request Mar 8, 2022

mustache-2.4.0 failed to build in Stackage Nightly JustusAdam/mustache#58

Closed

juhp mentioned this pull request Mar 9, 2022

avro-0.6.0.1 failed to build in Stackage Nightly haskell-works/avro#177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Lift instances #343

Add Lift instances #343

treeowl commented Dec 21, 2021 •

edited

Loading

phadej Dec 21, 2021

treeowl Dec 21, 2021

phadej Dec 21, 2021 •

edited

Loading

phadej Dec 21, 2021

treeowl Dec 21, 2021

phadej Dec 21, 2021

sjakobi left a comment

sjakobi Dec 21, 2021

treeowl Dec 21, 2021

sjakobi Dec 21, 2021

treeowl Dec 21, 2021

sjakobi Dec 21, 2021

madgen Dec 21, 2021

treeowl Dec 21, 2021

phadej Dec 21, 2021

treeowl Dec 21, 2021

treeowl commented Dec 21, 2021

treeowl commented Dec 22, 2021

treeowl commented Dec 22, 2021

madgen commented Dec 23, 2021

madgen commented Dec 26, 2021

madgen commented Dec 26, 2021

treeowl commented Dec 26, 2021

madgen commented Dec 26, 2021 •

edited

Loading

treeowl commented Dec 26, 2021

phadej commented Dec 26, 2021 •

edited

Loading

madgen commented Dec 26, 2021

phadej commented Dec 26, 2021

treeowl commented Dec 26, 2021

madgen commented Dec 26, 2021

Add Lift instances #343

Add Lift instances #343

Conversation

treeowl commented Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phadej Dec 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

treeowl commented Dec 21, 2021

treeowl commented Dec 22, 2021

treeowl commented Dec 22, 2021

madgen commented Dec 23, 2021

madgen commented Dec 26, 2021

madgen commented Dec 26, 2021

treeowl commented Dec 26, 2021

madgen commented Dec 26, 2021 • edited Loading

treeowl commented Dec 26, 2021

phadej commented Dec 26, 2021 • edited Loading

madgen commented Dec 26, 2021

phadej commented Dec 26, 2021

treeowl commented Dec 26, 2021

madgen commented Dec 26, 2021

treeowl commented Dec 21, 2021 •

edited

Loading

phadej Dec 21, 2021 •

edited

Loading

madgen commented Dec 26, 2021 •

edited

Loading

phadej commented Dec 26, 2021 •

edited

Loading