Unique as special boxing type & hidden constructors
Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
The definition dates back to 1996, so it seems plausible that newtype is the way to go now. Edward Excerpts from p.k.f.holzenspies's message of 2014-08-15 11:52:47 +0100:
Dear all,
I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler.
Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no?
2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place?
I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible.
Regards,
Philip
FastInt = Int#, so newtype doesn't work here. Cheers, Simon On 15/08/2014 14:01, Edward Z. Yang wrote:
The definition dates back to 1996, so it seems plausible that newtype is the way to go now.
Edward
Excerpts from p.k.f.holzenspies's message of 2014-08-15 11:52:47 +0100:
Dear all,
I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler.
Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no?
2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place?
I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible.
Regards,
Philip
ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs
Newtype of Int, to be clear. Edward Excerpts from Simon Marlow's message of 2014-09-04 11:49:39 +0200:
FastInt = Int#, so newtype doesn't work here.
Cheers, Simon
On 15/08/2014 14:01, Edward Z. Yang wrote:
The definition dates back to 1996, so it seems plausible that newtype is the way to go now.
Edward
Excerpts from p.k.f.holzenspies's message of 2014-08-15 11:52:47 +0100:
Dear all,
I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler.
Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no?
2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place?
I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible.
Regards,
Philip
ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs
Dear Simon, The point is to have newtype Unique = Unique Int where we use the boxing of Int, instead of creating our own boxing. Actually, it seems useful to move to newtype Unique = Unique Word (see other discussions about unnecessary signedness). I've been working on this (although only as a side-project, so progress is very slow) and I've discovered a lot of API-out-of-sync-ness; there are comments stating we don't export mkUnique, so that we can keep track of all the Chars we use. Unfortunately, we *do* export mkUnique from Unique and we do *not* have consistent use of Chars everywhere. I'm working to replace the Char-mechanism with a (rather straightforward) sum-type UniqueDomain. This should also help get a more consistent treatment of serialisation (the one in the module Unique is *slightly* different from the one in BinIface). I'm still not quite sure how to do the performance tests on the actual compilation (i.e. runtime of GHC itself). If anything, moving Uniques to a higher abstraction (coerced boxed values, instead of manually boxed stuff) is actually a good litmus test of how far GHC's optimisations have come since '96 ;) If you have any more input, especially on performance stuff (what would be the worst acceptable performance hit and measured on what, for example), it would be *very* welcome! Regards, Philip ________________________________________ From: Simon Marlow <marlowsd@gmail.com> Sent: 04 September 2014 11:49 To: Edward Z. Yang; Holzenspies, P.K.F. (EWI) Cc: ghc-devs Subject: Re: Unique as special boxing type & hidden constructors FastInt = Int#, so newtype doesn't work here. Cheers, Simon On 15/08/2014 14:01, Edward Z. Yang wrote:
The definition dates back to 1996, so it seems plausible that newtype is the way to go now.
Edward
Excerpts from p.k.f.holzenspies's message of 2014-08-15 11:52:47 +0100:
Dear all,
I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler.
Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no?
2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place?
I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible.
Regards,
Philip
ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs
On 04/09/2014 13:25, p.k.f.holzenspies@utwente.nl wrote:
Dear Simon,
The point is to have
newtype Unique = Unique Int
Yes, I misunderstood, thanks!
where we use the boxing of Int, instead of creating our own boxing. Actually, it seems useful to move to
newtype Unique = Unique Word
(see other discussions about unnecessary signedness).
I've been working on this (although only as a side-project, so progress is very slow) and I've discovered a lot of API-out-of-sync-ness; there are comments stating we don't export mkUnique, so that we can keep track of all the Chars we use. Unfortunately, we *do* export mkUnique from Unique and we do *not* have consistent use of Chars everywhere. I'm working to replace the Char-mechanism with a (rather straightforward) sum-type UniqueDomain. This should also help get a more consistent treatment of serialisation (the one in the module Unique is *slightly* different from the one in BinIface).
I'm still not quite sure how to do the performance tests on the actual compilation (i.e. runtime of GHC itself). If anything, moving Uniques to a higher abstraction (coerced boxed values, instead of manually boxed stuff) is actually a good litmus test of how far GHC's optimisations have come since '96 ;)
If you have any more input, especially on performance stuff (what would be the worst acceptable performance hit and measured on what, for example), it would be *very* welcome!
There are compiler performance tests in testsuite/tests/perf/compiler. They will only fail if the performance gets worse by 10% or so (depending on the test), so instead of checking for failure just run the tests manually and compare the allocation values before and after your change (remember to save a copy of the original compiler binary so that you can do comparisons). Another way to test performance is to compile a large module, e.g. nofib/spectral/simple/Main.hs is a good one. Cheers, Simon
Regards, Philip
________________________________________ From: Simon Marlow <marlowsd@gmail.com> Sent: 04 September 2014 11:49 To: Edward Z. Yang; Holzenspies, P.K.F. (EWI) Cc: ghc-devs Subject: Re: Unique as special boxing type & hidden constructors
FastInt = Int#, so newtype doesn't work here.
Cheers, Simon
On 15/08/2014 14:01, Edward Z. Yang wrote:
The definition dates back to 1996, so it seems plausible that newtype is the way to go now.
Edward
Excerpts from p.k.f.holzenspies's message of 2014-08-15 11:52:47 +0100:
Dear all,
I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler.
Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no?
2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place?
I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible.
Regards,
Philip
ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs
Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I'd be ok with that change. Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only *make* a unique from an Int by calling 'mkUniqueGrimily', which signals clearly that something fishy is going on. And rightly so! Simon From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of p.k.f.holzenspies@utwente.nl Sent: 15 August 2014 11:53 To: ghc-devs@haskell.org Subject: Unique as special boxing type & hidden constructors Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
Dear Simon, et al, Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts. 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated). 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin. I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code). Regards, Philip ________________________________ Van: Simon Peyton Jones <simonpj@microsoft.com> Verzonden: maandag 18 augustus 2014 00:11 Aan: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org Onderwerp: RE: Unique as special boxing type & hidden constructors Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I’d be ok with that change. Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only *make* a unique from an Int by calling ‘mkUniqueGrimily’, which signals clearly that something fishy is going on. And rightly so! Simon From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of p.k.f.holzenspies@utwente.nl Sent: 15 August 2014 11:53 To: ghc-devs@haskell.org Subject: Unique as special boxing type & hidden constructors Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
PS. Unique also looks like a case where Ints are used and (>= 0) is asserted. Can these cases be converted to Word as per earlier discussions? ________________________________ Van: p.k.f.holzenspies@utwente.nl <p.k.f.holzenspies@utwente.nl> Verzonden: maandag 18 augustus 2014 15:49 Aan: simonpj@microsoft.com; ghc-devs@haskell.org Onderwerp: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts. 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated). 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin. I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code). Regards, Philip ________________________________ Van: Simon Peyton Jones <simonpj@microsoft.com> Verzonden: maandag 18 augustus 2014 00:11 Aan: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org Onderwerp: RE: Unique as special boxing type & hidden constructors Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I’d be ok with that change. Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only *make* a unique from an Int by calling ‘mkUniqueGrimily’, which signals clearly that something fishy is going on. And rightly so! Simon From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of p.k.f.holzenspies@utwente.nl Sent: 15 August 2014 11:53 To: ghc-devs@haskell.org Subject: Unique as special boxing type & hidden constructors Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? I think so, unless others yell to the contrary. 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
OK by me 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Please don’t change this. If you want to change how pretty-printing of uniques works, and want to find all the call sites of pprUnique, it’s FAR easier to grep for pprUnique than to search for all calls of ppr, and work out which are at type Unique! (In my view) it’s usually much better not to use type classes unless you actually need overloading. Simon From: p.k.f.holzenspies@utwente.nl [mailto:p.k.f.holzenspies@utwente.nl] Sent: 18 August 2014 14:50 To: Simon Peyton Jones; ghc-devs@haskell.org Subject: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts. 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated). 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin. I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code). Regards, Philip ________________________________ Van: Simon Peyton Jones <simonpj@microsoft.com<mailto:simonpj@microsoft.com>> Verzonden: maandag 18 augustus 2014 00:11 Aan: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Onderwerp: RE: Unique as special boxing type & hidden constructors Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I’d be ok with that change. Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only *make* a unique from an Int by calling ‘mkUniqueGrimily’, which signals clearly that something fishy is going on. And rightly so! Simon From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl> Sent: 15 August 2014 11:53 To: ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: Unique as special boxing type & hidden constructors Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
Dear Simon, et al, I seem to recall that the Unique(Supply) was an issue in parallelising GHC itself. There's a comment in the code (signed JSM) that there aren't any 64-bit bugs, if we have at least 32-bits for Ints and Chars fit in 8 characters. Then, there's bitmasks like 0x00FFFFFF to separate the "Int-part" from the "Char-part". I was wondering; if we move Uniques to 64 bits, but use the top 16 (instead of the current 8) for *both* the tag (currently a Char, soon an sum-type) and the threadId of the supplying thread of a Unique, would that help? Regards, Philip ________________________________ From: Simon Peyton Jones <simonpj@microsoft.com> Sent: 18 August 2014 23:29 To: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org Subject: RE: Unique as special boxing type & hidden constructors 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? I think so, unless others yell to the contrary. 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
OK by me 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Please don’t change this. If you want to change how pretty-printing of uniques works, and want to find all the call sites of pprUnique, it’s FAR easier to grep for pprUnique than to search for all calls of ppr, and work out which are at type Unique! (In my view) it’s usually much better not to use type classes unless you actually need overloading. Simon From: p.k.f.holzenspies@utwente.nl [mailto:p.k.f.holzenspies@utwente.nl] Sent: 18 August 2014 14:50 To: Simon Peyton Jones; ghc-devs@haskell.org Subject: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts. 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated). 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin. ? I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code). Regards, Philip ________________________________ Van: Simon Peyton Jones <simonpj@microsoft.com<mailto:simonpj@microsoft.com>> Verzonden: maandag 18 augustus 2014 00:11 Aan: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Onderwerp: RE: Unique as special boxing type & hidden constructors Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I’d be ok with that change. Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only *make* a unique from an Int by calling ‘mkUniqueGrimily’, which signals clearly that something fishy is going on. And rightly so! Simon From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl> Sent: 15 August 2014 11:53 To: ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: Unique as special boxing type & hidden constructors Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
Sounds like a good idea to me. Would need to think about making sure that it all still worked, somehow, on 32 bit. S From: p.k.f.holzenspies@utwente.nl [mailto:p.k.f.holzenspies@utwente.nl] Sent: 20 August 2014 11:31 To: Simon Peyton Jones; ghc-devs@haskell.org Subject: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, I seem to recall that the Unique(Supply) was an issue in parallelising GHC itself. There's a comment in the code (signed JSM) that there aren't any 64-bit bugs, if we have at least 32-bits for Ints and Chars fit in 8 characters. Then, there's bitmasks like 0x00FFFFFF to separate the "Int-part" from the "Char-part". I was wondering; if we move Uniques to 64 bits, but use the top 16 (instead of the current 8) for *both* the tag (currently a Char, soon an sum-type) and the threadId of the supplying thread of a Unique, would that help? Regards, Philip ________________________________ From: Simon Peyton Jones <simonpj@microsoft.com<mailto:simonpj@microsoft.com>> Sent: 18 August 2014 23:29 To: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: RE: Unique as special boxing type & hidden constructors 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? I think so, unless others yell to the contrary. 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
OK by me 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Please don’t change this. If you want to change how pretty-printing of uniques works, and want to find all the call sites of pprUnique, it’s FAR easier to grep for pprUnique than to search for all calls of ppr, and work out which are at type Unique! (In my view) it’s usually much better not to use type classes unless you actually need overloading. Simon From: p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl> [mailto:p.k.f.holzenspies@utwente.nl] Sent: 18 August 2014 14:50 To: Simon Peyton Jones; ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts. 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated). 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin. I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code). Regards, Philip ________________________________ Van: Simon Peyton Jones <simonpj@microsoft.com<mailto:simonpj@microsoft.com>> Verzonden: maandag 18 augustus 2014 00:11 Aan: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Onderwerp: RE: Unique as special boxing type & hidden constructors Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I’d be ok with that change. Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only *make* a unique from an Int by calling ‘mkUniqueGrimily’, which signals clearly that something fishy is going on. And rightly so! Simon From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl> Sent: 15 August 2014 11:53 To: ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: Unique as special boxing type & hidden constructors Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
Methinks a lot of the former performance considerations in Unique are out-dated (as per earlier discussion; direct use of unboxed ints etc.). An upside of using an ADT for the types of uniques is that we don't actually need to reserve 8 bits for a Char (which is committing to neither the actual number of classes, nor the "nature" of real Chars in Haskell). Instead, we can make a bitmask dependent on the number of classes that we actually use and stick the tag on the least-significant side of the Unique, as opposed to the most-significant (as we do now). We want to keep things working on 32-bits, but maybe a future of parallel builds is only for 64-bits. In this case, I would suggest that the 64-bit-case looks like this: <thread_id_bits:8> <unique_id_bits:56-X> <tag_bits:X> whereas the 32-bit case simply has <unique_id_bits:32-X> <tag_bits:X> Where X is dependent on the size of the UniqueClass-sum-type (to be introduced). This would be CPP-magic'd using WORD_SIZE_IN_BITS. Ph. ________________________________ From: Simon Peyton Jones <simonpj@microsoft.com> Sent: 20 August 2014 13:01 To: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org Subject: RE: Unique as special boxing type & hidden constructors Sounds like a good idea to me. Would need to think about making sure that it all still worked, somehow, on 32 bit. S From: p.k.f.holzenspies@utwente.nl [mailto:p.k.f.holzenspies@utwente.nl] Sent: 20 August 2014 11:31 To: Simon Peyton Jones; ghc-devs@haskell.org Subject: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, I seem to recall that the Unique(Supply) was an issue in parallelising GHC itself. There's a comment in the code (signed JSM) that there aren't any 64-bit bugs, if we have at least 32-bits for Ints and Chars fit in 8 characters. Then, there's bitmasks like 0x00FFFFFF to separate the "Int-part" from the "Char-part". I was wondering; if we move Uniques to 64 bits, but use the top 16 (instead of the current 8) for *both* the tag (currently a Char, soon an sum-type) and the threadId of the supplying thread of a Unique, would that help? Regards, Philip ________________________________ From: Simon Peyton Jones <simonpj@microsoft.com<mailto:simonpj@microsoft.com>> Sent: 18 August 2014 23:29 To: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: RE: Unique as special boxing type & hidden constructors 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? I think so, unless others yell to the contrary. 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
OK by me 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Please don’t change this. If you want to change how pretty-printing of uniques works, and want to find all the call sites of pprUnique, it’s FAR easier to grep for pprUnique than to search for all calls of ppr, and work out which are at type Unique! (In my view) it’s usually much better not to use type classes unless you actually need overloading. Simon From: p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl> [mailto:p.k.f.holzenspies@utwente.nl] Sent: 18 August 2014 14:50 To: Simon Peyton Jones; ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: RE: Unique as special boxing type & hidden constructors Dear Simon, et al, Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts. 1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed? 2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated). 3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin. I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code). Regards, Philip ________________________________ Van: Simon Peyton Jones <simonpj@microsoft.com<mailto:simonpj@microsoft.com>> Verzonden: maandag 18 augustus 2014 00:11 Aan: Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Onderwerp: RE: Unique as special boxing type & hidden constructors Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I’d be ok with that change. Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only *make* a unique from an Int by calling ‘mkUniqueGrimily’, which signals clearly that something fishy is going on. And rightly so! Simon From: ghc-devs [mailto:ghc-devs-bounces@haskell.org] On Behalf Of p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl> Sent: 15 August 2014 11:53 To: ghc-devs@haskell.org<mailto:ghc-devs@haskell.org> Subject: Unique as special boxing type & hidden constructors Dear all, I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler. Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no? 2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place? I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible. Regards, Philip
On Wed, Aug 20, 2014 at 1:47 PM, <p.k.f.holzenspies@utwente.nl> wrote:
Methinks a lot of the former performance considerations in Unique are out-dated (as per earlier discussion; direct use of unboxed ints etc.).
An upside of using an ADT for the types of uniques is that we don't actually need to reserve 8 bits for a Char (which is committing to neither the actual number of classes, nor the "nature" of real Chars in Haskell). Instead, we can make a bitmask dependent on the number of classes that we actually use and stick the tag on the least-significant side of the Unique, as opposed to the most-significant (as we do now).
We want to keep things working on 32-bits, but maybe a future of parallel builds is only for 64-bits. In this case, I would suggest that the 64-bit-case looks like this:
<thread_id_bits:8> <unique_id_bits:56-X> <tag_bits:X>
Is the thread id deterministic between runs? If not, please do not use this layout. I remember vaguely Unique being relevant to ghc not having deterministic builds, my most wanted ghc feature: https://ghc.haskell.org/trac/ghc/ticket/4012 Alexander
whereas the 32-bit case simply has
<unique_id_bits:32-X> <tag_bits:X>
Where X is dependent on the size of the UniqueClass-sum-type (to be introduced). This would be CPP-magic'd using WORD_SIZE_IN_BITS.
Ph.
------------------------------ *From:* Simon Peyton Jones <simonpj@microsoft.com> *Sent:* 20 August 2014 13:01
*To:* Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org *Subject:* RE: Unique as special boxing type & hidden constructors
Sounds like a good idea to me. Would need to think about making sure that it all still worked, somehow, on 32 bit.
S
*From:* p.k.f.holzenspies@utwente.nl [mailto:p.k.f.holzenspies@utwente.nl]
*Sent:* 20 August 2014 11:31 *To:* Simon Peyton Jones; ghc-devs@haskell.org *Subject:* RE: Unique as special boxing type & hidden constructors
Dear Simon, et al,
I seem to recall that the Unique(Supply) was an issue in parallelising GHC itself. There's a comment in the code (signed JSM) that there aren't any 64-bit bugs, if we have at least 32-bits for Ints and Chars fit in 8 characters. Then, there's bitmasks like 0x00FFFFFF to separate the "Int-part" from the "Char-part".
I was wondering; if we move Uniques to 64 bits, but use the top 16 (instead of the current 8) for *both* the tag (currently a Char, soon an sum-type) and the threadId of the supplying thread of a Unique, would that help?
Regards,
Philip
------------------------------
*From:* Simon Peyton Jones <simonpj@microsoft.com> *Sent:* 18 August 2014 23:29 *To:* Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org *Subject:* RE: Unique as special boxing type & hidden constructors
1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed?
I think so, unless others yell to the contrary.
2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
OK by me
3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Please don’t change this. If you want to change how pretty-printing of uniques works, and want to find all the call sites of pprUnique, it’s FAR easier to grep for pprUnique than to search for all calls of ppr, and work out which are at type Unique!
(In my view) it’s usually much better not to use type classes unless you actually need overloading.
Simon
*From:* p.k.f.holzenspies@utwente.nl [mailto:p.k.f.holzenspies@utwente.nl <p.k.f.holzenspies@utwente.nl>] *Sent:* 18 August 2014 14:50 *To:* Simon Peyton Jones; ghc-devs@haskell.org *Subject:* RE: Unique as special boxing type & hidden constructors
Dear Simon, et al,
Looking at Unique, there are a few more design choices that may be outdated, and since I'm polishing things now, anyway, I figured I could update it on more fronts.
1) There is a #ifdef define(__GLASGOW_HASKELL__), which confused me somewhat. Similar things occur elsewhere in the code. Isn't the assumption that GHC is being used? Is this old portability stuff that may be removed?
2) Uniques are produced from a Char and an Int. The function to build Uniques (mkUnique) is not exported, according to the comments, so as to see all characters used. Access to these different "classes" of Uniques is given through specialised mkXXXUnique functions. Does anyone have a problem with something like:
data UniqueClass
= UniqDesugarer
| UniqAbsCFlattener
| UniqSimplStg
| UniqNativeCodeGen
...
and a public (i.e. exported) function:
mkUnique :: UniqueClass -> Int -> Unique
? The benefit of this would be to have more (to my taste) self-documenting code and a greater chance that documentation is updated (the list of "unique supply characters" in the comments is currently outdated).
3) Is there a reason for having functions implementing class-methods to be exported? In the case of Unique, there is pprUnique and:
instance Outputable Unique where
ppr = pprUnique
Here pprUnique is exported and it is used in quite a few places where it's argument is unambiguously a Unique (so it's not to force the type) *and* "ppr" is used for all kinds of other types. I'm assuming this is an old choice making things marginally faster, but I would say cleaning up the API / namespace would now outweigh this margin.
I will also be adding Haddock-comments, so when this is done, a review would be most welcome (I'll also be doing some similar transformations to other long-since-untouched-code).
Regards,
Philip
------------------------------
*Van:* Simon Peyton Jones <simonpj@microsoft.com> *Verzonden:* maandag 18 augustus 2014 00:11 *Aan:* Holzenspies, P.K.F. (EWI); ghc-devs@haskell.org *Onderwerp:* RE: Unique as special boxing type & hidden constructors
Re (1) I think this is a historical. A newtype wrapping an Int should be fine. I’d be ok with that change.
Re (2), I think your question is: why does module Unique export the data type Unique abstractly, rather than exporting both the data type and its constructor. No deep reason here, but it guarantees that you can only * *make** a unique from an Int by calling ‘mkUniqueGrimily’, which signals clearly that something fishy is going on. And rightly so!
Simon
*From:* ghc-devs [mailto:ghc-devs-bounces@haskell.org <ghc-devs-bounces@haskell.org>] *On Behalf Of * p.k.f.holzenspies@utwente.nl *Sent:* 15 August 2014 11:53 *To:* ghc-devs@haskell.org *Subject:* Unique as special boxing type & hidden constructors
Dear all,
I'm working with Alan to instantiate everything for Data.Data, so that we can do better SYB-traversals (which should also help newcomers significantly to get into the GHC code base). Alan's looking at the AST types, I'm looking at the basic types in the compiler.
Right now, I'm looking at Unique and two questions come up:
data Unique = MkUnique FastInt
1) As someone already commented: Is there a specific reason (other than history) that this isn't simply a newtype around an Int? If we're boxing anyway, we may as well use the default Int boxing and newtype-coerce to the specific purpose of Unique, no?
2) As a general question for GHC hacking style; what is the reason for hiding the constructors in the first place?
I understand about abstraction and there are reasons for hiding, but there's a "public GHC API" and then there are all these modules that people can import at their own peril. Nothing is guaranteed about their consistency from version to version of GHC. I don't really see the point about hiding constructors (getting in the way of automatically deriving things) and then giving extra functions like (in the case of Unique):
getKeyFastInt (MkUnique x) = x
mkUniqueGrimily x = MkUnique (iUnbox x)
I would propose to just make Unique a newtype for an Int and making the constructor visible.
Regards,
Philip
_______________________________________________ ghc-devs mailing list ghc-devs@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs
On Wed, Aug 20, 2014 at 1:47 PM, <p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl>> wrote: <thread_id_bits:8> <unique_id_bits:56-X> <tag_bits:X> Is the thread id deterministic between runs? If not, please do not use this layout. I remember vaguely Unique being relevant to ghc not having deterministic builds, my most wanted ghc feature: https://ghc.haskell.org/trac/ghc/ticket/4012 I think this depends on the policy GHC *will* have (there is not parallel build atm) wrt. the forking of threads. An actual Control.Concurrent.ThreadId might be as large as 64 bits, so, of course, we won't be using that, but rather the sequence number in which the UniqueSupply was "split off" for a new thread. In other words, if the decision to fork threads is deterministic, so are the Uniques with this layout. Mind you, I imagine a parallel GHC would still have at most one thread working on a single module. I don't know too much about what makes it into the interface file of a module (I can't imagine the exact Uniques end up there, because they would overlap with other modules - with per-module compilation - and conflict that way). Can you comment on how (the layout of) Uniques relate to #4012 in a little more detail? It seems that if the Uniques that somehow end up in the interface files could simply be stripped of the thread id, in which case, the problem reduces to the current one. Ph.
On Wed, Aug 20, 2014 at 3:07 PM, <p.k.f.holzenspies@utwente.nl> wrote:
On Wed, Aug 20, 2014 at 1:47 PM, <p.k.f.holzenspies@utwente.nl> wrote:
<thread_id_bits:8> <unique_id_bits:56-X> <tag_bits:X>
Is the thread id deterministic between runs? If not, please do not use this layout. I remember vaguely Unique being relevant to ghc not having deterministic builds, my most wanted ghc feature:
https://ghc.haskell.org/trac/ghc/ticket/4012
I think this depends on the policy GHC *will* have (there is not parallel build atm) wrt. the forking of threads. An actual Control.Concurrent.ThreadId might be as large as 64 bits, so, of course, we won't be using that, but rather the sequence number in which the UniqueSupply was "split off" for a new thread. In other words, if the decision to fork threads is deterministic, so are the Uniques with this layout.
Mind you, I imagine a parallel GHC would still have at most one thread working on a single module. I don't know too much about what makes it into the interface file of a module (I can't imagine the exact Uniques end up there, because they would overlap with other modules - with per-module compilation - and conflict that way).
Can you comment on how (the layout of) Uniques relate to #4012 in a little more detail? It seems that if the Uniques that somehow end up in the interface files could simply be stripped of the thread id, in which case, the problem reduces to the current one.
I frankly don't know. I just think it's better to keep ThreadId out of data that can bleed into symbols and what not. As you can see, the thread id is just a counter, and as forkIO in a threaded runtime will be racy between threads, they aren't deterministic. http://stackoverflow.com/questions/24995262/how-can-i-build-a-threadid-given... Alexander
Dear Max, et al, Here's hoping either you are still on the mailing list, or the address I found on your website (which says you're a Ph.D. student, so it's starting to smell) is still operational. I'm working on redoing some Unique-stuff in GHC. Mostly, the code uses Unique's API in a well-behaved fashion. The only awkward bit I found is in BinIface.getSymtabName, which git blames you for ;) I just wanted to ask: Why does this functions do all the bit-masking and shifting stuff directly and with different masks than anything in Unique? Is there a reason why this doesn't use unpkUnique? The comments in Unique state that mkUnique is NOT EXPORTED (the caps are in the comments, I'm not shouting), but they are, it seems, specifically for BinIface. I would like to get rid of this, but dare not hack away in the dark. Regards, Philip ________________________________ From: Alexander Kjeldaas <alexander.kjeldaas@gmail.com> Sent: 20 August 2014 15:48 To: Holzenspies, P.K.F. (EWI) Cc: Simon Peyton Jones; ghc-devs Subject: Re: Unique as special boxing type & hidden constructors On Wed, Aug 20, 2014 at 3:07 PM, <p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl>> wrote: On Wed, Aug 20, 2014 at 1:47 PM, <p.k.f.holzenspies@utwente.nl<mailto:p.k.f.holzenspies@utwente.nl>> wrote: <thread_id_bits:8> <unique_id_bits:56-X> <tag_bits:X> Is the thread id deterministic between runs? If not, please do not use this layout. I remember vaguely Unique being relevant to ghc not having deterministic builds, my most wanted ghc feature: https://ghc.haskell.org/trac/ghc/ticket/4012 I think this depends on the policy GHC *will* have (there is not parallel build atm) wrt. the forking of threads. An actual Control.Concurrent.ThreadId might be as large as 64 bits, so, of course, we won't be using that, but rather the sequence number in which the UniqueSupply was "split off" for a new thread. In other words, if the decision to fork threads is deterministic, so are the Uniques with this layout. Mind you, I imagine a parallel GHC would still have at most one thread working on a single module. I don't know too much about what makes it into the interface file of a module (I can't imagine the exact Uniques end up there, because they would overlap with other modules - with per-module compilation - and conflict that way). Can you comment on how (the layout of) Uniques relate to #4012 in a little more detail? It seems that if the Uniques that somehow end up in the interface files could simply be stripped of the thread id, in which case, the problem reduces to the current one. I frankly don't know. I just think it's better to keep ThreadId out of data that can bleed into symbols and what not. As you can see, the thread id is just a counter, and as forkIO in a threaded runtime will be racy between threads, they aren't deterministic. http://stackoverflow.com/questions/24995262/how-can-i-build-a-threadid-given... Alexander
participants (5)
-
Alexander Kjeldaas -
Edward Z. Yang -
p.k.f.holzenspies＠utwente.nl -
Simon Marlow -
Simon Peyton Jones