Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more instances. #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TravisWhitaker
Copy link
Contributor

@TravisWhitaker TravisWhitaker commented Sep 21, 2019

Adds instances for:

  • UArray
  • ForeignPtr
  • TVar

Fixes #44 and fixes #15. Note that the ForeignPtr instance is strict in the contained Addr# but not the ForeignPtrContents. I think this is fine, since the only pure computation that can be done on a ForeignPtr is address arithmetic.

Copy link
Member

@RyanGlScott RyanGlScott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Can you also add an entry to the changelog.md about this?

Control/DeepSeq.hs Outdated Show resolved Hide resolved
Control/DeepSeq.hs Outdated Show resolved Hide resolved
Control/DeepSeq/BackDoor.hs Outdated Show resolved Hide resolved
Control/DeepSeq.hs Outdated Show resolved Hide resolved
Control/DeepSeq.hs Outdated Show resolved Hide resolved
Control/DeepSeq.hs Outdated Show resolved Hide resolved
Control/DeepSeq.hs Outdated Show resolved Hide resolved
@RyanGlScott
Copy link
Member

Great, thanks @TravisWhitaker.

@hvr, does this look good to you?

@TravisWhitaker
Copy link
Contributor Author

Pinging @hvr

@TravisWhitaker
Copy link
Contributor Author

What is required to merge this?

@RyanGlScott
Copy link
Member

I haven't merged this yet since I was reminded of an ongoing discussion about whether deepseq should provide NFData for "reference-like" types (such IORef and (->)) at all. @hvr (the principal maintainer of deepseq) has expressed skepticism about this, so I'm hesitant to proceed further without his input.

FWIW, the only instances in this patch that would fall into the "reference-like" category are the TVar and ForeignPtr instances. The UArray instance seems uncontroversial, so perhaps that should be in its own patch.

@TravisWhitaker
Copy link
Contributor Author

I don't see how the instances for TVar and ForeignPtr (and IORef) are anything like an instance for (->) r. All of the former types are explicit references; there's no good reason IMO to expect that a normal form reference can only refer to normal form data.

Precisely what it means to have a normal form value of type (->) r seems murky to me. What it means to have a normal form value of type TVar, Ptr, ForeignPtr, STRef, or IORef seems pretty straight forward in contrast: there's no thunk evaluation required to determine where the reference is pointing.

@cartazio
Copy link

cartazio commented Nov 1, 2019 via email

@TravisWhitaker
Copy link
Contributor Author

I think the point is that for most other structures the definitions of the provided instances pass through to the child types.

What's a "child type"? If it's just a type constructor's argument(s), would you consider the instances for Ptr, FunPtr, Fixed, StableName, MVar, Proxy, :~:, and :~~: problematic too?

@TravisWhitaker
Copy link
Contributor Author

@RyanGlScott @hvr what are the next steps for this PR?

@TravisWhitaker
Copy link
Contributor Author

Pinging @RyanGlScott @hvr how can this be moved forward?

@SamuelSchlesinger
Copy link

I think the point is that for most other structures the definitions of the provided instances pass through to the child types. If we had some sort of non recursive marker new type say NonRec we could perhaps have instances like NonRec (Ioref a)?

I like this approach. One could define NFRec and NonNFRec wrappers and the relevant instances in a completely separate package, as the maintainers of this library seem potentially uninterested in this PR moving forward at the moment.

@cartazio
Copy link

cartazio commented Dec 27, 2019 via email

@cartazio
Copy link

@andrewthad @chessai thoughts?

@chessai
Copy link
Member

chessai commented Dec 29, 2019

I didn't see this, sorry. I like the wrapper idea. I think the addition of those, plus expanding documentation would work well and minimise breakage. I am specifically talking about reference types, not (->).

@cartazio
Copy link

ok, so would we do eg

newtype NoRecWHNF a = NRWHNF a
instance NFData (NoRecWHNF a) where
   rnf (NRWHNF a) = rwhnf a 

and say "use this"?

@chessai
Copy link
Member

chessai commented Dec 29, 2019

Yep, that's what i was thinking of. The added NFData instances (different from what's in this PR) would go to NF (including the inner type), but the NoRec would always just go to WHNF. Are we on the same page?

@cartazio
Copy link

Ok. So you’re proposing that nfdata instances should only be provided for things we can fully normalize (which perhaps includes flat structures like storable or boxed vectors.) and anything else should go through a marker that says “sorry, only whnf, no recursion”?

Or could you unpack the intent and execution and present vs future state I think you’re proposing?

@TravisWhitaker
Copy link
Contributor Author

I'm confused about what the Rec/NoRec wrappers have to do with the instances proposed in this PR. UArray, ForeignPtr, and TVar are not recursive types, and although their type constructors take arguments, whether or not values of type UArray i a/ForeignPtr a/TVar a are in normal form have nothing to do with what the argument type a is, or indeed any values of type a. These types have no a in their structures at all; for example, I can easily construct a value of type ForeignPtr Void that is in normal form.

I've been working with this definition of "normal form:" values in normal form can not be reduced any further, i.e.

  • The constructor is determined.
  • The constructor's arguments are not thunks.
  • The constructor's arguments are in normal form.

This is subtle in the case of some reference types, since the reference's constructors don't contain the referenced type. Consider ForeignPtr for example. Values of type ForeignPtr a may reference values of type a, even though the ForeignPtr constructor and its arguments don't hold a value of type a. Because a ForeignPtr a is really just an Addr# with some finalizers, we can use the exact same normalization strategy no matter what a is. The normalized values yielded by the instances in this PR are not in WHNF even though rwhnf is a sufficient implementation (except for ForeignPtr, which is strict in the Addr# but not the finalizers). A ForeignPtr with a known Addr# is a real normal form value, unlike, for example, the values yielded by the NFData (a -> b) instance (although I might be wrong about this, since I don't know what a function's normal form is) . Since this is a strategy that yields correct normal forms, I don't think it warrants a special wrapper.

It would be a mistake to dereference the a that a ForeignPtr might point to (e.g. nullPtr can be made into a ForeignPtr), since the referenced value has nothing to do with whether or not a ForeignPtr value is in normal form. Indeed, I have a library that would segfault if the NFData (ForeignPtr a) instance attempted to dereference and normalize the referenced a values.

I suspect that Herbert simply hasn't had the time to review this PR or participate in this conversation. I'll shoot him an email, since that seems to be his preference these days.

@TravisWhitaker
Copy link
Contributor Author

The added NFData instances (different from what's in this PR) would go to NF (including the inner type)

@chessai, wouldn't the "recursive" NFData a => NFData (IORef a) instance require unsafePerformIO or similar? What rnf definition would you use for this instance?

@cartazio
Copy link

cartazio commented Dec 30, 2019 via email

@chessai
Copy link
Member

chessai commented Dec 30, 2019

@TravisWhitaker yeah, my mistake. Didn't think too deeply on what I said about the added instances. And for most reference types, what i suggested doesn't really make any sense, as you pointed out. Perhaps the PR is just best as it stands, with sufficient documentation. To my understanding, we aren't really going to move forward without Herbert though.

Perhaps your well-worded note about reference types should be in the documentation, if that is how we will treat them?

values in normal form can not be reduced any further, i.e.
• The constructor is determined.
• The constructor's arguments are not thunks.
• The constructor's arguments are in normal form.

This is subtle in the case of some reference types, since the reference's constructors don't contain the referenced type. Consider ForeignPtr for example. Values of type ForeignPtr a may reference values of type a , even though the ForeignPtr constructor and its arguments don't hold a value of type a . Because a ForeignPtr a is really just an Addr# with some finalizers, we can use the exact same normalization strategy no matter what a is. The normalized values yielded by the instances in this PR are not in WHNF even though rwhnf is a sufficient implementation (except for ForeignPtr, which is strict in the Addr# but not the finalizers). A ForeignPtr with a known Addr# is a real normal form value, unlike, for example, the values yielded by the NFData (a -> b) instance.

EDIT: the note needs an edit, of course, if it were to be introduced into the actual documentation

@TravisWhitaker
Copy link
Contributor Author

Pinging @hvr .

@cartazio
Copy link

cartazio commented Feb 28, 2020 via email

@TravisWhitaker
Copy link
Contributor Author

Pinging @hvr.

@RyanGlScott is there anything that can be done to move this forward? I'm really curious to hear Herbert's take on this, but it's rather unfortunate for something like this to be bottlenecked for months on one person.

@RyanGlScott
Copy link
Member

I'm no longer on the CLC, so I can't really say one way or the other.

@TravisWhitaker
Copy link
Contributor Author

@sjakobi @chessai what more can be done to either:

  • Move this forward?
  • Clearly explain and document why this should not be done?

For what it's worth, I expanded on my line of reasoning for the existence of these instances here.

@sjakobi
Copy link
Member

sjakobi commented Aug 27, 2021

@TravisWhitaker I'm not a maintainer of this package.

@chessai
Copy link
Member

chessai commented Sep 1, 2021

HVR is unavailable. Pinging @bgamari and @ekmett and @emilypi and @gwils for thoughts.

@mixphix
Copy link
Collaborator

mixphix commented Dec 5, 2021

@TravisWhitaker I've just dropped support for GHC 7 to simplify the CPP in Control.DeepSeq. Would you please rebase so that these instances can finally be added?

@TravisWhitaker
Copy link
Contributor Author

@mixphix in light of c2432c2, I removed the UArray instance from this PR, since the design intent seems to be for array to depend on deepseq instead.

@mixphix
Copy link
Collaborator

mixphix commented Nov 11, 2024

Thanks for that, that is indeed the intent. Also since deepseq no longer supports versions of GHC with base <= 4.8 (corresponding to GHC < 8), the CPP you've included is unnecessary, and the import can simply be import Foreign.ForeignPtr.Safe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add instance for ForeignPtr Instance for TVar
7 participants