Make `Version` type opaque #3905

hvr · 2016-09-27T21:31:46Z

Similiar to dabd9d9 which made
PackageName opaque, this makes Distribution.Version.Version opaque.

The most common version numbers occuring on Hackage are 3- and
4-component version. This results in significant Heap overhead due to
Data.Version's inefficient internal representation.

So like the PackageName commit, this commit is a preparatory commit to
pave the way for replacing Version's internal representation by a
representation with a memory footprint which can be an order of
magnitude smaller.

phadej · 2016-09-27T21:48:40Z

Cabal/Distribution/Version.hs

+data Version = Version [Int]
+             deriving (Data,Eq,Ord,Generic,Show,Read,Typeable)
+
+instance Binary Version


Should we say it explicitly we aren't going to preserve Binary compatibility (pun unintended)

yeah, that makes sense; I already mention that Data.Version and D.V.Version are distinct types, so I'd just have to point out that Binary instances cannot be expected to be compatible either.

phadej · 2016-09-27T21:49:42Z

Cabal/Distribution/Version.hs

+unVersion :: Version -> [Int]
+unVersion (Version vs) = vs
+
+-- | Constant representing the /null' 'Version'


formatting?

23Skidoo · 2016-09-27T21:52:51Z

Haven't read carefully, but +1 from me in general.

23Skidoo · 2016-09-27T21:58:28Z

BTW, this also means that we can make the Eq and Ord instances more sane (see #1368).

hvr · 2016-09-27T22:13:47Z

@23Skidoo unfortunately such Eq/Ord instances would violate obvious laws such as 'x == y' --> 'f x == f y' for all 'f'; the weird Eq/Ord instances were iirc also the reason why versionTags are being deprecated.

So if we want more sane Eq/Ord instances, we have to forget the trailing zeros in such a way that they can't be observed anymore.

23Skidoo · 2016-09-27T22:33:21Z

Hmm, the substitution law is not obvious actually. Usually in mathematics an equivalence relation is only required to be symmetric, reflexive and transitive.

23Skidoo · 2016-09-27T22:41:38Z

So if we want more sane Eq/Ord instances, we have to forget the trailing zeros in such a way that they can't be observed anymore.

Well, we can't do that because people want to see them. So show will necessarily break the substitution law.

Usually in mathematics an equivalence relation is only required to be symmetric, reflexive and transitive.

The substitution law does indeed seem to be controversial, see https://www.reddit.com/r/haskell/comments/1njlqr/laws_for_the_eq_class/.

ezyang · 2016-09-27T22:41:50Z

@hvr Well, if we make 0.8 and 0.8.0 equivalent, with a #1368 Eq/Ord instance, your law is essentially stating that all functions f must respect the equivalence relation that relates 0.8 and 0.8.0. If you have an appropriate interface which doesn't make it possible to distinguish these two numbers (e.g., always normalizes), then the law will hold.

ezyang · 2016-09-28T00:29:58Z

For this PR, I don't think we should change Eq/Ord. Leave that for a later patchset.

ezyang · 2016-09-28T00:30:46Z

Build failure is real! (Test with old version of GHC).

hvr · 2016-09-28T05:34:55Z

@23Skidoo I know that code in matrix.hho (and I bet in hackage-server as well) would break if the substitution/leibniz property wouldn't hold, and there's code I just recently added to cabal (guess which :-) ) which would result in subtle/confusing effects if Version had hidden state.

Similiar to dabd9d9 which made `PackageName` opaque, this makes `Distribution.Version.Version` opaque. The most common version numbers occuring on Hackage are 3- and 4-component version. This results in significant Heap overhead due to `Data.Version`'s inefficient internal representation. So like the `PackageName` commit, this commit is a preparatory commit to pave the way for replacing `Version`'s internal representation by a representation with a memory footprint which can be an order of magnitude smaller.

hvr · 2016-09-28T06:20:41Z

@ezyang fyi, I pushed a fix which I expect to fix all build failures; as a nice benefit, this refactoring allows us to kill all that CPP-hackery in .Version

@phadej I've also tweaked the haddocks

23Skidoo · 2016-09-28T07:00:23Z

@hvr

and there's code I just recently added to cabal (guess which :-) ) which would result in subtle/confusing effects if Version had hidden state.

The hash-consing code? Yes, that would break if there was both foo-1.0 and foo-1.0.0 in the index.

hvr · 2016-09-28T07:04:45Z

@23Skidoo

Yes, that would break if there was both foo-1.0 and foo-1.0.0 in the index.

unfortunately there are: #3515 (comment)

we did add code to Hackage though to make it possible at some more distant point in the future to ignore those (or maybe carefully manually remove those historic duplicate versions from the index): haskell/hackage-server@27f9727

ezyang · 2016-09-28T07:15:08Z

Needs changelog entry.

23Skidoo · 2016-09-28T07:16:36Z

@hvr Yeah, I remember, just couldn't find that ticket. I guess that we'll have to settle for using compare on normaliseVersion where appropriate, or add a NormalisedVersion newtype.

hvr · 2016-09-28T07:38:04Z

@23Skidoo @ezyang @phadej @dcoutts ...so, is the changelog entry the only thing missing before this PR can be merged?

dcoutts

Sorry to bikeshed, but naming is important for readability, and we use versions a lot.

So I suggest the follow renamings:

unVersion -> versionDigits
withVersion -> alterVersion
mkNullVersion -> nullVersion

and delete the existing nullVersion, replace the few places where it's used with == nullVersion.

On naming, I also suggest we use "digits" as the name of the thing that a version consists of, rather than the slightly strange name "branch" that we inherited from base Version (which contrasted branch vs tags).

So apart from doing that search and replace, there's only a few minor things I've commented on specifically like now-out-of-date comments or where we could use alterVersion.

dcoutts · 2016-09-28T09:09:33Z

Cabal/Distribution/Version.hs

+--
+-- @since 2.0
+unVersion :: Version -> [Int]
+unVersion (Version vs) = vs


I was thinking we should give this a name like versionDigits.

Once the internal rep changes, it'll be less clear that unVersion should refer to producing the abstract representation rather than the underlying physical representation.

Having read through the rest of the code I also think it'd just read a lot better / more clearly.

dcoutts · 2016-09-28T09:10:51Z

Cabal/Distribution/Version.hs

+
+-- functor-like helper
+withVersion :: ([Int] -> [Int]) -> Version -> Version
+withVersion f = mkVersion . f . unVersion


Would alterVersion be clearer? We tend to use withFoo for more IOish things.

dcoutts · 2016-09-28T09:12:08Z

Cabal/Distribution/Version.hs

+--
+-- @since 2.0
+nullVersion :: Version -> Bool
+nullVersion = null . unVersion


I fear we're going to get confused between nullVersion and mkNullVersion.

What if we just had nullVersion :: Version and for the test sites we used v == nullVersion?

dcoutts · 2016-09-28T09:14:42Z

Cabal/Distribution/Version.hs


 isWildcardRange :: Version -> Version -> Bool
-isWildcardRange (Version branch1 _) (Version branch2 _) = check branch1 branch2
+isWildcardRange ver1 ver2 = check (unVersion ver1) (unVersion ver2)


I think this is an example case where arguably it'd read nicer / more clearly as

isWildcardRange ver1 ver2 = check (versionDigits ver1) (versionDigits ver2)

dcoutts · 2016-09-28T09:16:47Z

Cabal/Distribution/Version.hs

-      []        -> [0,1] -- should not happen
-      [m1]      -> [m1,1] -- e.g. version '1'
-      (m1:m2:_) -> [m1,m2+1]
+majorUpperBound = withVersion $ \ver -> case ver of


alterVersion $ \digits -> case digits of

perhaps?

dcoutts · 2016-09-28T09:28:12Z

cabal-install/Distribution/Client/Init.hs

@@ -477,11 +476,11 @@ pvpize :: Version -> VersionRange
 pvpize v = orLaterVersion v'
           `intersectVersionRanges`
           earlierVersion (incVersion 1 v')
-  where v' = (v { versionBranch = take 2 (versionBranch v) })
+  where v' = mkVersion . take 2 . unVersion $ v


alterVersion

dcoutts · 2016-09-28T09:30:01Z

cabal-install/Distribution/Client/Setup.hs

@@ -2233,8 +2233,8 @@ parseDependencyOrPackageId = parse Parse.+++ liftM pkgidToDependency parse
  where
    pkgidToDependency :: PackageIdentifier -> Dependency
    pkgidToDependency p = case packageVersion p of
-      Version [] _ -> Dependency (packageName p) anyVersion
-      version      -> Dependency (packageName p) (thisVersion version)
+      v | nullVersion v -> Dependency (packageName p) anyVersion


this would be just as clear with v == nullVersion. So I think we could simplify.

dcoutts · 2016-09-28T09:30:53Z

cabal-install/tests/UnitTests/Distribution/Client/ArbitraryInstances.hs

 instance Arbitrary Version where
  arbitrary = do
    branch <- shortListOf1 4 $
                frequency [(3, return 0)
                          ,(3, return 1)
                          ,(2, return 2)
                          ,(1, return 3)]
-    return (Version branch []) -- deliberate []
+    return (mkVersion branch) -- deliberate []


Drop the (now out of date) comment.

dcoutts · 2016-09-28T09:31:50Z

cabal-install/tests/UnitTests/Distribution/Solver/Modular/DSL.hs

@@ -531,9 +530,9 @@ extractInstallPlan = catMaybes . map confPkg . CI.SolverInstallPlan.toList

    srcPkg :: SolverPackage UnresolvedPkgLoc -> (String, Int)
    srcPkg cpkg =
-      let C.PackageIdentifier pn (Version (n:_) _) =
+      let C.PackageIdentifier pn ver = -- (Version (n:_) _) =


why the comment?

dcoutts · 2016-09-28T09:34:05Z

Cabal/Distribution/Simple/SrcDist.hs

-                 ++ [dateToSnapshotNumber date]
-  }
+snapshotVersion date ver
+  = mkVersion (unVersion ver ++ [dateToSnapshotNumber date])


alterVersion

dcoutts · 2016-09-28T09:42:08Z

@hvr Oh and I forgot to say, I wholly approve of this! Very good :-)

hvr · 2016-09-28T11:44:53Z

@dcoutts I'm totally ok with all renamings, except for

On naming, I also suggest we use "digits" as the name of the thing that a version consists of, rather than the slightly strange name "branch" that we inherited from base Version (which contrasted branch vs tags)

Naming is important, hence do I cringe at calling something like "3001" a digit, which is really a number (or in the context of a "version number" a kind of sequence)

I've tried looking for prior art, but descriptions of version numbering schemes I could find almost never refer to version elements as "digits" as that's a quite ambigous term (c.f. "PVP uses a 2-digit major version" vs "PVP uses a 2-{component,part} major version", or "trailing zero digits" vs "trailing zero components"), some specifications even evade the issue of naming the subcomponents (c.f. semver.org) and calls them generically "identifiers", Microsoft tends talk about e.g. 4-part version numbers, IBM refers to integers, some other refer to the numbers a version is made up as number, which are all more accurate terms than "digit" imho.

At some level it would make sense to call them "version-digits" if version numbers were a positional numeral system, but version numbers are a different kind of mathematical structure since they're closer to a infinite-dimensional vectorspace, rather than a scalar, so the version components act rather like a coefficient to their respective basis-element, or like elements of a list or tuple, rather than a digit in a numeral...

So I'd suggest alternatives:

versionParts
versionComponents
versionIds
versionElements
versionNumbers
versionNumerals
versionToList
...

23Skidoo · 2016-09-28T12:28:01Z

My vote is for either versionNumbers or versionComponents.

dcoutts · 2016-09-28T13:00:45Z

Naming is important, hence do I cringe at calling something like "3001" a digit, which is really a number (or in the context of a "version number" a kind of sequence)

Ok, fair enough.

My vote is for either versionNumbers or versionComponents.

I'm happy with either of those, with a preference for versionNumbers.

dcoutts

LGTM. Squash back into one patch and merge when green!

dcoutts · 2016-09-28T15:37:36Z

Cabal/Distribution/Simple/SrcDist.hs

-    versionBranch = versionBranch version
-                 ++ [dateToSnapshotNumber date]
-  }
+snapshotVersion date = alterVersion (++ [dateToSnapshotNumber date])


dcoutts · 2016-09-28T15:40:02Z

Cabal/tests/UnitTests/Distribution/Version.hs

      maxPoint | null allVersionsUsed = minPoint
-               | otherwise = case maximum allVersionsUsed of
-                   Version vs _ -> Version (vs ++ [1]) []
+               | otherwise = mkVersion . (++[1]) . versionNumbers . maximum $ allVersionsUsed


alterVersion (++[1]) (maximum allVersionsUsed)

This optimises the `[Int]` representation to a 16-byte heap object for ~99% of version numbers (up to 4 components, each within a [0..0xfffe] value range) occuring on Hackage. One noteworthy improvement of this optimisation is a significant reduction of the size of the 01-index.cache file from previously 6299700 bytes (before haskell#3905) down to 3408864 bytes, i.e. down to ~54% Also, this reduces the memory footprint and GC overhead a bit for e.g. `cabal info zzz` (which reads in the index cache) from cabal.0: There is no package named 'zzz'. You may need to run 'cabal update' to get the latest list of available packages. 859,337,368 bytes allocated in the heap 447,261,128 bytes copied during GC 37,385,208 bytes maximum residency (19 sample(s)) 1,311,136 bytes maximum slop 103 MB total memory in use (0 MB lost due to fragmentation) Tot time (elapsed) Avg pause Max pause Gen 0 1613 colls, 0 par 0.268s 0.268s 0.0002s 0.0012s Gen 1 19 colls, 0 par 0.227s 0.227s 0.0119s 0.0506s TASKS: 4 (1 bound, 3 peak workers (3 total), using -N1) SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled) INIT time 0.001s ( 0.001s elapsed) MUT time 0.431s ( 0.758s elapsed) GC time 0.495s ( 0.495s elapsed) EXIT time 0.006s ( 0.005s elapsed) Total time 0.934s ( 1.259s elapsed) Alloc rate 1,991,870,623 bytes per MUT second Productivity 46.9% of total user, 34.8% of total elapsed to cabal.1: There is no package named 'zzz'. You may need to run 'cabal update' to get the latest list of available packages. 834,314,392 bytes allocated in the heap 440,791,176 bytes copied during GC 36,663,112 bytes maximum residency (19 sample(s)) 2,225,040 bytes maximum slop 96 MB total memory in use (0 MB lost due to fragmentation) Tot time (elapsed) Avg pause Max pause Gen 0 1574 colls, 0 par 0.254s 0.254s 0.0002s 0.0007s Gen 1 19 colls, 0 par 0.223s 0.223s 0.0118s 0.0474s TASKS: 4 (1 bound, 3 peak workers (3 total), using -N1) SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled) INIT time 0.001s ( 0.001s elapsed) MUT time 0.383s ( 0.699s elapsed) GC time 0.477s ( 0.477s elapsed) EXIT time 0.005s ( 0.005s elapsed) Total time 0.869s ( 1.182s elapsed) Alloc rate 2,175,866,164 bytes per MUT second Productivity 44.9% of total user, 33.0% of total elapsed

This optimises the `[Int]` representation to a 16-byte heap object for ~99% of version numbers (up to 4 components, each within a [0..0xfffe] value range) occuring on Hackage. One noteworthy improvement of this optimisation is a significant reduction of the size of the 01-index.cache file from previously 6299700 bytes (before #3905) down to 3408864 bytes, i.e. down to ~54% Also, this reduces the memory footprint and GC overhead a bit for e.g. `cabal info zzz` (which reads in the index cache) from cabal.0: There is no package named 'zzz'. You may need to run 'cabal update' to get the latest list of available packages. 859,337,368 bytes allocated in the heap 447,261,128 bytes copied during GC 37,385,208 bytes maximum residency (19 sample(s)) 1,311,136 bytes maximum slop 103 MB total memory in use (0 MB lost due to fragmentation) Tot time (elapsed) Avg pause Max pause Gen 0 1613 colls, 0 par 0.268s 0.268s 0.0002s 0.0012s Gen 1 19 colls, 0 par 0.227s 0.227s 0.0119s 0.0506s TASKS: 4 (1 bound, 3 peak workers (3 total), using -N1) SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled) INIT time 0.001s ( 0.001s elapsed) MUT time 0.431s ( 0.758s elapsed) GC time 0.495s ( 0.495s elapsed) EXIT time 0.006s ( 0.005s elapsed) Total time 0.934s ( 1.259s elapsed) Alloc rate 1,991,870,623 bytes per MUT second Productivity 46.9% of total user, 34.8% of total elapsed to cabal.1: There is no package named 'zzz'. You may need to run 'cabal update' to get the latest list of available packages. 834,314,392 bytes allocated in the heap 440,791,176 bytes copied during GC 36,663,112 bytes maximum residency (19 sample(s)) 2,225,040 bytes maximum slop 96 MB total memory in use (0 MB lost due to fragmentation) Tot time (elapsed) Avg pause Max pause Gen 0 1574 colls, 0 par 0.254s 0.254s 0.0002s 0.0007s Gen 1 19 colls, 0 par 0.223s 0.223s 0.0118s 0.0474s TASKS: 4 (1 bound, 3 peak workers (3 total), using -N1) SPARKS: 0 (0 converted, 0 overflowed, 0 dud, 0 GC'd, 0 fizzled) INIT time 0.001s ( 0.001s elapsed) MUT time 0.383s ( 0.699s elapsed) GC time 0.477s ( 0.477s elapsed) EXIT time 0.005s ( 0.005s elapsed) Total time 0.869s ( 1.182s elapsed) Alloc rate 2,175,866,164 bytes per MUT second Productivity 44.9% of total user, 33.0% of total elapsed

phadej reviewed Sep 27, 2016

View reviewed changes

hvr force-pushed the pr/opaque-version branch from e6b2b10 to 5f40eb0 Compare September 28, 2016 06:19

dcoutts requested changes Sep 28, 2016

View reviewed changes

23Skidoo mentioned this pull request Sep 28, 2016

Version 0.8.0 is different from 0.8 #1368

Open

Integrate requests from code-review

d63104a

dcoutts approved these changes Sep 28, 2016

View reviewed changes

minor tweak [skip ci]

6003aa9

hvr merged commit bb2026c into haskell:master Sep 28, 2016

hvr mentioned this pull request Sep 28, 2016

Optimise Version representation #3908

Merged

hvr deleted the pr/opaque-version branch October 2, 2016 08:28

hvr mentioned this pull request Jul 17, 2017

Upgrade to Cabal-2.0 haskell/hackage-server#600

Closed

Make Version type opaque #3905

Make Version type opaque #3905

Conversation

hvr commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

23Skidoo commented Sep 27, 2016

23Skidoo commented Sep 27, 2016

hvr commented Sep 27, 2016 • edited Loading

23Skidoo commented Sep 27, 2016

23Skidoo commented Sep 27, 2016 • edited Loading

ezyang commented Sep 27, 2016

ezyang commented Sep 28, 2016

ezyang commented Sep 28, 2016 • edited Loading

hvr commented Sep 28, 2016

hvr commented Sep 28, 2016

23Skidoo commented Sep 28, 2016

hvr commented Sep 28, 2016 • edited Loading

ezyang commented Sep 28, 2016

23Skidoo commented Sep 28, 2016

hvr commented Sep 28, 2016

dcoutts left a comment

Choose a reason for hiding this comment

dcoutts Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

dcoutts Sep 28, 2016 • 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

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

dcoutts commented Sep 28, 2016

hvr commented Sep 28, 2016 • edited Loading

23Skidoo commented Sep 28, 2016

dcoutts commented Sep 28, 2016

dcoutts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `Version` type opaque #3905

Make `Version` type opaque #3905

hvr commented Sep 27, 2016 •

edited

Loading

23Skidoo commented Sep 27, 2016 •

edited

Loading

ezyang commented Sep 28, 2016 •

edited

Loading

hvr commented Sep 28, 2016 •

edited

Loading

dcoutts Sep 28, 2016 •

edited

Loading

dcoutts Sep 28, 2016 •

edited

Loading

hvr commented Sep 28, 2016 •

edited

Loading