#9163 closed bug (fixed)

Ptr should have a phantom role

Reported by: simonpj Owned by:
Priority: normal Milestone: 7.10.1
Component: Compiler Version: 7.8.2
Keywords: Cc: simonmar, core-libraries-committee@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: roles/should_compile/Roles2
Blocked By: Blocking:
Related Tickets: #9164 Differential Revisions:

Description

In GHC.Ptr we see

type role Ptr representational
data Ptr a = Ptr Addr# deriving (Eq, Ord)

with no comments. Why is Ptr representational?

In the same module we have castPtr:

castPtr :: Ptr a -> Ptr b
castPtr (Ptr addr) = Ptr addr

which unpacks and repacks a Ptr. If Ptr was phantom, we could use coerce. And that in turn would actually make a lot of code more efficient – there are lots of calls to castPtr. Specifically, in nofib, I tried implementing castPtr with unsafeCoerce. Then I found:

  • 12% less allocation in reverse-complem
  • 7.3% less allocation in fasta.
  • Binary sizes fell 0.1%.

Both these benchmarks are ones that do a lot of I/O, and it turns out that GHC.IO.Handle.Text.$wa1 has a castPtr that (all by itself) accounts for 12% of reverse-complem's total allocation! So making castPtr free is good.

Change History (29)

comment:1 Changed 15 months ago by simonpj

Richard says: It seemed clear to me that Ptr should be representational, thinking that we don't want to coerce a Ptr Int to a Ptr Bool. I don't see any reason why this couldn't be changed.

Why is Ptr a data not a newtype? If it were a newtype, we could keep the role annotation and use coerce internally, according to the updated Coercible solver. Answer (from Simon): because the payload is an unboxed Addr#, so Ptr boxes it. A newtype can't have an unboxed payload.

However, it is crucial that FunPtr have a representational argument, as normaliseFfiType' depends on this fact. There is a brief comment in TcForeign, but clearly more comments are necessary. Will do shortly. If we want to change FunPtr's role, normaliseFfiType' would have to be updated, too, but it shouldn't be hard.

comment:2 Changed 15 months ago by simonpj

Austin says: I think Ptr should almost certainly be representational, as it is a case where the actual underlying value is the same, but what it points to is not (I'll ignore castPtr here for a second).

This makes me think of something I talked about before with Andres - in general, phantom roles seem somewhat dangerous, and I kind of wonder if they should be inferred by default. Often you see some code along the lines of:

module A ( Bar, newBarInt ) where

data Bar a = Bar Int

newBarInt :: Int -> Bar Int

where A is exported to the client and the module boundary enforces some restrictions on 'Bar', like what types we can instantiate Bar a to (the example is dumb but bear with me).

In the above example, I believe the a in Bar a would be inferred at phantom role.

The question I have is: what legitimate case would there be for phantom roles like this? Such usage of phantom type parameters seems very common, but in almost *all* cases I can't think of a reason why I would ever want a user to be allowed to coerce away the type information, if the a parameter was phantom. It seems like in the above example, I would almost certainly want a to be representational instead.

What other cases exist for legitimate phantom roles such as this where we want this inference? I wonder if instead we should require an explicit role annotation for phantoms in this case - not the other way around.

Ptr is a similar story - by default it would maybe seem reasonable to be phantom, but that seems like an especially grievous error, given we don't want people to poke incorrect values in or something (again, ignoring castPtr for a second, but I think the general idea holds.)

comment:3 Changed 15 months ago by simonpj

Well, remember that even if Ptr has a phantom role, the two types (Ptr Int) and (Ptr Double) would be different types. It's just that you could *coerce* between them. Which is perhaps not unreasonable. Indeed that is precisely what castPtr does. It’s not silent or implicit.

I still don't see why it would be bad to make Ptr phantom.

comment:4 Changed 15 months ago by nomeata

Slightly off topic, but maybe GHC.IO.Handle.Text can be written so that castPtr is not needed. It seems it always works with Ptr Word8, but exposes an API that uses Ptr a as a generic pointer. The type a is never needed inside GHC.IO.Handle.Text...

comment:5 follow-up: Changed 15 months ago by goldfire

While I hear Austin's concerns, I tend to agree with Simon here. We don't want to consider Ptr Int and Ptr Bool to be the same types, but we do want to allow a knowledgeable programmer to get from one to the other for free. This seems to be a great use case for coerce. This idea extends to other cases where GHC infers phantom roles.

In case it got lost, I'll ask again: why is Ptr a datatype, not a newtype?

comment:6 in reply to: ↑ 5 Changed 15 months ago by simonpj

Replying to goldfire

In case it got lost, I'll ask again: why is Ptr a datatype, not a newtype?

See my reply in comment 1

comment:7 Changed 15 months ago by simonpj

  • Cc simonmar added

Adding Simon Marlow who may want to comment on comment 4.

comment:8 Changed 15 months ago by nomeata

I tried it, and hPutBuf & Co are called with Ptr Word8 (sometimes called LitString) in all cases but BufWrite.bPutCStringLen, where we have a Ptr CChar, where CChar is a newtype. So by using coerce there (and unsafeCoerce during bootstrapping), we get the best of boths worlds: The more restrictive role and the free behaviour.

comment:9 Changed 15 months ago by simonpj

Well regardless of the decision about the role of Ptr, what you describe sounds like an improvement anyway, getting rid of (always suspicious) castPtr calls. If you grep in the base library you'll find quite a few others. Can you eliminate them in the same way?

I think this would be a nice improvement, if you cared to push it through.

Simon

comment:10 Changed 15 months ago by rwbarton

+1 from me on making Ptr's role phantom. But especially I don't think it should be exactly representational; either phantom or nominal must be better. A Ptr a is not actually a pointer to a Haskell heap object of type a, for any type a. Neither the representation of the Ptr a itself nor the representation of the thing-being-pointed-to depends on the representation of a specifically. I might define a newtype BigEndianInt = BigEndianInt Int and write a Storable instance that uses a big-endian format when peeking or pokeing. Then it isn't any more valid to cast from Ptr Int to Ptr BigEndianInt than it is to cast from Ptr Int to Ptr Double.

Similar comments probably apply to Austin's hypothetical example of an inferred phantom role. If the library author does not want Bar to have a phantom role, they probably do not want it to have a representational role either. But it's hard to say without a more fledged-out example.

comment:11 follow-up: Changed 15 months ago by goldfire

Helpful comments, rwbarton. Is the same true for FunPtr? I would assume so.

comment:12 in reply to: ↑ 11 Changed 15 months ago by rwbarton

Replying to goldfire:

Helpful comments, rwbarton. Is the same true for FunPtr? I would assume so.

My knee-jerk reaction is "not sure"; I am thinking that while the normal way to consume a Ptr is to peek and poke it, the normal way to consume a FunPtr is with a foreign import ccall "dynamic" wrapper, and I'm not sure they are obviously analogous situations.

comment:13 Changed 15 months ago by simonmar

So I've been assuming that castPtr was free all this time, and it turns out it isn't! Please make it free.

We can't change the type of hPutBuf and friends without potentially breaking code. There's no better choice than Ptr a here, because we don't know the type of the underlying data. Making it Ptr Word8 would just force the caller to use castPtr sometimes without any benefit. e.g. Ptr CChar is also a sensible choice, but it could in reality be anything.

It does look like the two castPtrs in bufWrite are unnecessary, though.

comment:14 Changed 15 months ago by nomeata

Discussion about castPtr moved to #9164.

comment:15 Changed 15 months ago by nomeata

I agree with rwbarton: The Ptr’s parameter is, in a sense, a convenience for the user. The API could just use Ptr without an argument, and users could use Tagged a Ptr if they want to track types. By that reasoning, Ptr’s argument should be phantom, as nothing in Foreign.Ptr takes a into account.

comment:16 Changed 15 months ago by simonpj

OK so

  • I think we have consensus that Ptr should have phantom role. Joachim, could you push that through?
  • Am am not sure what the story is on FunPtr. Maybe it doesn't matter for now; but if we leave it as it is, could we add a comment to the role signature saying that it's a conservative choice, and pointing to this ticket?

It'd be nice to get this done and closed.

Thanks

Simon

comment:17 Changed 15 months ago by Joachim Breitner <mail@…>

In 1946922c61df427e59f8a00572fd4dd6501abd98/ghc:

Make Ptr's parameter phantom

and implement castPtr with coerce, which gives
    12% less allocation in reverse-complem
    7.3% less allocation in fasta.
    Binary sizes fell 0.1%.
as reported and discussed in #9163.

comment:18 Changed 15 months ago by nomeata

  • Resolution set to fixed
  • Status changed from new to closed

comment:19 Changed 15 months ago by simonmar

I think it would be good to do this for FunPtr too, if it's not too hard.

comment:20 Changed 15 months ago by nomeata

  • Resolution fixed deleted
  • Status changed from closed to new

I had to revert the patch, adding NoImplicitPrelude to Data.Coerce makes the build system trip over.

comment:21 Changed 15 months ago by simonpj

  • Cc core-libraries-committee@… added

FWIW I'd also be happy to treat FunPtr just like Ptr (ie phantom).

I'll add the core-libraries-committee in cc

Simon

comment:22 Changed 15 months ago by Joachim Breitner <mail@…>

In 5bdbd510a78f0c17d702fa9399cc0501cfd00fac/ghc:

Make Ptr's parameter phantom

and implement castPtr with coerce, which gives
    12% less allocation in reverse-complem
    7.3% less allocation in fasta.
    Binary sizes fell 0.1%.
as reported and discussed in #9163.

comment:23 Changed 15 months ago by goldfire

  • Owner set to goldfire

As noted previously, FunPtr's roles are a little fiddlier. I'll take this one, and add a bunch of comments while I'm at it.

comment:24 Changed 15 months ago by goldfire

  • Test Case set to roles/should_compile/Roles2

Patch on the way...

comment:25 Changed 15 months ago by Richard Eisenberg <eir@…>

In 9e6c6b4206cd893434e49cd893eb67081eeffe99/ghc:

Make FunPtr's role be phantom; add comments.

This change also updates castFunPtr to make it free at runtime.
This fixes #9163.

comment:26 Changed 15 months ago by goldfire

  • Milestone set to 7.8.4
  • Status changed from new to merge

I believe this closes this ticket. Suggesting merging because of related performance improvement, but it's not critical.

comment:27 Changed 15 months ago by goldfire

  • Owner goldfire deleted
  • Status changed from merge to new

Can't disown a "merge" ticket. Reopening to disown.

comment:28 Changed 15 months ago by goldfire

  • Status changed from new to merge

comment:29 Changed 14 months ago by thoughtpolice

  • Milestone changed from 7.8.4 to 7.10.1
  • Resolution set to fixed
  • Status changed from merge to closed

I'm leaving these as is out of 7.8.3; I don't think they're critical, and some of the refactorings in base to Data.Coerce mean the original patches don't quite apply cleanly.

Note: See TracTickets for help on using tickets.