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

!!! project causes memory inconsistencies #38

Open
Profpatsch opened this issue Nov 11, 2022 · 10 comments
Open

!!! project causes memory inconsistencies #38

Profpatsch opened this issue Nov 11, 2022 · 10 comments

Comments

@Profpatsch
Copy link

Profpatsch commented Nov 11, 2022

project (and potentially also inject, we haven’t verified`, will cause undefined behavior, which we noticed & verified in a production system.

The bug only appears with optimizations enabled (only tested -O2), not in ghci

I don’t have a minimal repro (yet), but I can describe what we did:

There was one function which loaded some data in IO, constructed a ist of rec, then before returning used project to reduce unnecessary fields in every list element:

loadData :: IO (Record '[ "foo" := Text ])
loadData = do
  listOfRes <- <fetch data>
  pure 
    $ map 
         (project @'["foo" := Text, "bar" := Int] @'["foo" := Text])
         listOfRes

And another function that pulled a subset of fields from the record, like this:

requestBody = do
  dat <- loadData
  
  let mapDat d = (Json.object [
         ("foo", get #foo d)
        ])
        
   pure $ Json.Array (map mapDat dat)

Let’s say the final data was

[ 
  { foo = "xx" },
  { foo = "yy" },
  { foo = "zz" },
]

Then what would be actually returned was (!!):

[ 
  { foo = "xx" },
  { foo = "xx" },
  { foo = "xx" },
]

now, if you rewrote the function to deeply evaluate the projected record:

loadData :: IO (Record '[ "foo" := Text ])
loadData = do
  listOfRes <- <fetch data>

  print listOfRes   -- <- deeply evaluate the record by printing it
 
  pure 
    $ map 
         (project @'["foo" := Text, "bar" := Int] @'["foo" := Text])
         listOfRes

It would again return

[ 
  { foo = "xx" },
  { foo = "yy" },
  { foo = "zz" },
]

When we rewrote the original function to construct a new record instead of projecting:

loadData :: IO (Record '[ "foo" := Text ])
loadData = do
  listOfRes <- <fetch data>
  pure 
    $ map 
         (\res -> rcons #foo (get #foo res) rnil)
         listOfRes

The problem went away.

@Profpatsch
Copy link
Author

cc @sheaf who wrote project a while ago

@sheaf
Copy link
Contributor

sheaf commented Nov 11, 2022

I don't recall the exact details, but I think I saw an issue like this start happening with GHC 9.0 or 9.2. The issue went away with -fno-full-laziness. What version of GHC are you using, and does the problem still happen with -fno-full-laziness?

@Profpatsch
Copy link
Author

Yes, that fixes it … but why, that’s horrifying

@Profpatsch
Copy link
Author

How can every entry in a list of records pulled out of IO be changed by laziness floating.

@sheaf
Copy link
Contributor

sheaf commented Nov 11, 2022

The relevant GHC issue is #19413 (although that is for runRW# not runST). See in particular my comment. This started happening with GHC 9.0.

@Profpatsch
Copy link
Author

I’m trying to reproduce the problem in the test suite, but I don’t know how to trigger the mis-optimization

085540b

@Profpatsch
Copy link
Author

Also cabal test flat-out refuses to use the -O2 from the cabal file, you have to run cabal test -O2

@Profpatsch
Copy link
Author

@sheaf Do you by any chance have any time next week to create a minimal reproduction example for the GHC bug tracker?

@Profpatsch
Copy link
Author

@sheaf I meant doing a video call to figure one out based on the unsafePerformIO example @ndmitchell gives in https://gitlab.haskell.org/ghc/ghc/-/issues/19413

@sheaf
Copy link
Contributor

sheaf commented Nov 23, 2022

@Profpatsch Could you try replacing, purely in this library,

runST' :: (forall s. ST s a) -> a
runST' !s = runST s

with

import GHC.ST (ST(..))
import GHC.Exts (runRW#, lazy)

runST' :: (forall s. ST s a) -> a
runST' !(ST st) = case runRW# st of (# _, a #) -> lazy a

If that works, I think that would be good evidence we need the same lazy trick for runST.

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

No branches or pull requests

2 participants