WIP: Revive compact representation for NP/NS #129

edsko · 2021-01-14T09:01:32Z

This is an attempt to revive https://github.com/well-typed/generics-sop/tree/compact-representation .

phadej · 2021-01-14T10:42:55Z

sop-core/sop-core.cabal

@@ -1,5 +1,5 @@
 name:                sop-core
-version:             0.5.0.1
+version:             0.5.1.0


FWIW, this is definitely not a non-breaking change.

OTOH, staged-streams compiles fine. (Plenty of warnings about non-exhautive pattern matches on NS, but those are not fatal).

Yeah, my hope was that with the pattern synonym it would be backwards compatible; and those warnings should even be absent from 8.10.

The data structure is now stricter (spine strict?). This is contrived example, but still, the behavior changes observably:

Currently,

hd $ I 'x' :* (error "ay, caramba!" :: NP I '[]) I 'x'

With this patch

*Data.SOP Data.SOP.NP> hd $ I 'x' :* (error "ay, caramba!" :: NP I '[]) I *** Exception: ay, caramba! CallStack (from HasCallStack): error, called at <interactive>:21:16 in interactive:Ghci1

Ah, fair enough. Hadn't thought terribly hard about strictness just yet :)

Is there a good reason why someone might rely on the construction of the NP being lazy? the idea of writing code which might only look at the first N elements of a NP seems pretty odd to me.

FWIW, I think that NP and NS should be spine-strict, and not making them so from the beginning was a mistake. Before sop-core, I'd have said there's basically no reason not to make the change. Now, it's theoretically possible there are uses that aren't generic programming related which might have other requirements. But it may be worth just doing it.

phadej · 2021-01-14T11:27:54Z

sop-core/src/Data/SOP/NS.hs

-deriving instance All (Eq   `Compose` f) xs => Eq   (NS f xs)
-deriving instance (All (Eq `Compose` f) xs, All (Ord `Compose` f) xs) => Ord (NS f xs)
+instance All (Show `Compose` f) xs => Show (NS f xs) where
+  show ns @ (NS i _) =


Add TODO to implement this properly. To be backward compatible this needs to print Ss and Zs.

BTW. The space between @ and (NS ... will break on GHC-9.0 due ghc-proposals/ghc-proposals#229

src/Data/SOP/NS.hs:197:3: error: Parse error in pattern: show In a function binding for the ‘@’ operator. Perhaps you meant an as-pattern, which must not be surrounded by whitespace | 197 | show ns @ (NS i _) =

(Otherwise this patch is GHC-9.0 friendly)

axman6 · 2021-01-18T16:26:59Z

I just came to repo to see if it would be possible to have a more compact representation of NS, could a similar thing be done using

data NS (f :: k -> Type) (ts :: [k]) = NS Word Any

I’m less sure if it would have as much of a performance improvement as the rest of this PR, but it would definitely improve memory usage.

edsko · 2021-01-19T07:47:58Z

Yes, that's certainly possible. The original branch from which I was working does in fact do that.

For my current purposes however, it turns out this representation is still not quite good enough, so I'm not sure if this work will go anywhere right now.

axman6 · 2021-01-19T08:09:36Z

Ah great, I found the branch and it looks like it was a little more complicated than I expected (I had started playing with an implementation of the idea this afternoon but ran into some of the issues that the original branch ran into, such a making instances for Show, Eq etc. work)

I would be really keen to see some benchmarks before and after with the original, your PR's work, the compact NS implementation, and both (though I understand if you aren't keen do the last two).

edsko · 2021-01-19T08:10:31Z

Perhaps @kosmikus did some on the original compact representation branch?

kosmikus · 2021-02-22T15:23:14Z

I did only do runtime benchmarking, and I wasn't too impressed. I think staging is the far superior approach for runtime performance. Compile-time is a different question though.

If the goal is that any compact-representation branch would become the default at any point, it needs thorough benchmarking though. I don't want to replace a safe implementation with a potentially unsafe one for questionable / not noticeable gain.

kosmikus

I'm basically convinced we should finally do this. There's still a lingering question whether we should also offer the old inductive versions in sop-core. Advantages could be:

somewhat easier to do performance comparisons if both are available in a single package
other use cases of sop-core might need to use e.g. :* for construction in a now less efficient way

My remarks are basically points for discussion. I'm happy to make the changes myself, once we reach consensus.

The more important point is that this is only partial though, and we need to make changes to the other functions before merging this. While it should all be correct as it currently is, because the old constructors are faithfully reimplemented, many functions can and should be much more efficiently implemented in terms of the underlying compact representation.

kosmikus · 2021-04-10T07:15:12Z

sop-core/src/Data/SOP/NP.hs

-data NP :: (k -> Type) -> [k] -> Type where
-  Nil  :: NP f '[]
-  (:*) :: f x -> NP f xs -> NP f (x ': xs)
+newtype NP (f :: k -> *) (xs :: [k]) = NP (V.Vector Any)


I'm still unconvinced that Any is better than f Any here. Do you have any data points on this?

I started out with just Any on my branch, despite having seen you use f Any, because I didn't see the point. Then later on I realized that actually having that f there is really rather helpful; often that f is concrete (say, an applicative functor or something) and having, say, f (Any -> Any) and f Any in hand is a lot more helpful than Any and Any.

(Well, maybe not concrete concrete, but at least we know something about the structure.)

kosmikus · 2021-04-10T07:17:47Z

sop-core/src/Data/SOP/NP.hs


+#if __GLASGOW_HASKELL__ >= 802
+{-# COMPLETE Nil, (:*) #-}
+#endif


We don't need this CPP anymore, as 8.0 is the oldest GHC version we support even now, but in practice, we're probably going to shift the lower bound to 8.6 soon.

kosmikus · 2021-04-10T07:21:50Z

sop-core/src/Data/SOP/NP.hs

 module Data.SOP.NP
  ( -- * Datatypes
-    NP(..)
+    NP(.., Nil, (:*))


Do we want to export the NP constructor, or do we want to move it to an Internal module?

Moving it would make sense to me, otherwise there are bound to be uses of NP that use the patterns dictionary and they would now suddenly get much worse efficiency.

I would definitely want to be able to access the NP constructor as a developer, but putting it in a .Internal modules makes sense as it doesn't provide a safe interface, which Nil and (:*) do. If I know what I'm doing and performance matters to me, I'd want to be able to work with the Vector directly and convince myself I did so safely.

kosmikus · 2021-04-10T07:23:01Z

sop-core/src/Data/SOP/NP.hs

+             => forall x xs . (xs' ~ (x ': xs)) => f x -> NP f xs -> NP f xs'
+pattern x :* xs <- (viewNP -> IsCons x xs)
+  where
+    x :* NP xs = NP (V.cons (unsafeCoerce x) xs)


The should be a documentation warning here that the complexity of this for construction is O(n).

kosmikus · 2021-04-10T07:24:15Z

sop-core/src/Data/SOP/NS.hs

-data NS :: (k -> Type) -> [k] -> Type where
-  Z :: f x -> NS f (x ': xs)
-  S :: NS f xs -> NS f (x ': xs)
+data NS (f :: k -> *) (xs :: [k]) = NS !Int Any


Same remark as before. Why not f Any?

We probably should add an UNPACK pragma for good measure.

kosmikus · 2021-04-10T07:26:38Z

sop-core/src/Data/SOP/NS.hs

 module Data.SOP.NS
  ( -- * Datatypes
-    NS(..)
+    NS(.., Z, S)


Same remark as for NP regarding the export of the NS constructor.

kosmikus · 2021-04-10T07:32:12Z

sop-core/src/Data/SOP/NS.hs

-    go (fs :* _  ) (Z xs ) = Z (ap_NP fs  xs )
-    go (_  :* fss) (S xss) = S (go    fss xss)
-    go Nil         x       = case x of {}
+    go (NP nps) (NS i ns) = NS i (unsafeCoerce ap_NS (nps V.! i) ns)


Why not inline go here?

kosmikus · 2021-04-10T20:09:15Z

FWIW, I've started working on porting the rest of the old compact-representation branch.

ghost · 2021-10-27T05:16:21Z

Is there any word on progress of this PR?

ghost

I noticed a missing optimisation based on the new representation. Is there any chance this PR will actually get merged? It's been stagnant for a few months now.

ghost · 2021-12-07T23:51:13Z

sop-core/src/Data/SOP/NS.hs

+unZ (NS _ x) = unsafeCoerce x

 -- | Obtain the index from an n-ary sum.
 --


Commenting here for the index_NS definition below, but this PR should also have:

index_NS :: forall f xs . NS f xs -> Int index_NS (NS i _) = i

Revive compact representation of NP/NS

cd9f2ef

edsko force-pushed the edsko/compact-representation branch from 107facc to cd9f2ef Compare January 14, 2021 09:29

phadej reviewed Jan 14, 2021

View reviewed changes

kosmikus reviewed Apr 10, 2021

View reviewed changes

ghost reviewed Dec 7, 2021

View reviewed changes

ghost mentioned this pull request Dec 17, 2021

Investigate compilation performance using large-records brendanhay/amazonka#717

Open

amesgen mentioned this pull request Apr 15, 2025

Define a custom Cardano-like era with long TLL for benchmarking it IntersectMBO/ouroboros-consensus#1464

Open

WIP: Revive compact representation for NP/NS #129

Are you sure you want to change the base?

WIP: Revive compact representation for NP/NS #129

Uh oh!

Conversation

edsko commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phadej Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axman6 commented Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

edsko commented Jan 19, 2021

Uh oh!

axman6 commented Jan 19, 2021

Uh oh!

edsko commented Jan 19, 2021

Uh oh!

kosmikus commented Feb 22, 2021

Uh oh!

kosmikus left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosmikus commented Apr 10, 2021

Uh oh!

ghost commented Oct 27, 2021

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

phadej Jan 14, 2021 •

edited

Loading

axman6 commented Jan 18, 2021 •

edited

Loading

kosmikus left a comment •

edited

Loading