darcs

Patch 1934 add -fno-cse to D.P.Prim.Named given the unsafePerformIO

Title add -fno-cse to D.P.Prim.Named given the unsafePerformIO
Superseder Nosy List ganesh
Related Issues
Status accepted Assigned To
Milestone

Created on 2019-09-22.22:33:29 by ganesh, last changed 2020-01-30.18:39:52 by bfrk.

Files
File name Status Uploaded Type Edit Remove
add-_fno_cse-to-d_p_prim_named-given-the-unsafeperformio.dpatch ganesh, 2019-09-22.22:33:29 application/x-darcs-patch
add-_fno_cse-to-d_p_prim_named-given-the-unsafeperformio.dpatch ganesh, 2020-01-25.22:23:52 application/x-darcs-patch
patch-preview.txt ganesh, 2019-09-22.22:33:29 text/x-darcs-patch
patch-preview.txt ganesh, 2020-01-25.22:23:52 text/x-darcs-patch
unnamed ganesh, 2019-09-22.22:33:29 text/plain
unnamed ganesh, 2020-01-25.22:23:52 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg21538 (view) Author: ganesh Date: 2019-09-22.22:33:29
I won't screen this immediately. It's technically
correct, but maybe not worth the noise given we hope
to drop fromAnonymousPrim completely. And none of our
other unsafePerformIOs have it.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 957497857baceec7e129ac4cff61eedc27dca863
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sun Sep 22 21:51:31 BST 2019
  * add -fno-cse to D.P.Prim.Named given the unsafePerformIO
Attachments
msg21546 (view) Author: bfrk Date: 2019-09-23.12:14:09
> I won't screen this immediately. It's technically
> correct, but maybe not worth the noise given we hope
> to drop fromAnonymousPrim completely. And none of our
> other unsafePerformIOs have it.

I was wondering about that. The docs also say something about disabling
full laziness optimization...
msg21550 (view) Author: ganesh Date: 2019-09-23.13:06:34
-no-full-laziness isn't needed in this particular case
because the unsafePerformIO can't float outside the lambda -
its body uses a variable bound by the function.
msg21553 (view) Author: bfrk Date: 2019-09-23.16:38:38
> -no-full-laziness isn't needed in this particular case
> because the unsafePerformIO can't float outside the lambda -
> its body uses a variable bound by the function.

I am glad you feel confident to say that. I have read the paragraph in
the docs and didn't really understand what that one was about...
msg21554 (view) Author: ganesh Date: 2019-09-23.16:52:29
I think the example helps.

  f x = unsafePerformIO (newIORef [])

needs the flag because otherwise the compiler might turn it into something
like

  f = let v = unsafePerformIO (newIORef []) in \x -> v

But if you have

  f x = unsafePerformIO (newIORef [x])

it's ok because the x on the right hand side makes that transformation 
invalid.
msg21563 (view) Author: bfrk Date: 2019-09-23.21:55:13
> I think the example helps.
> 
>   f x = unsafePerformIO (newIORef [])
> 
> needs the flag because otherwise the compiler might turn it into something
> like
> 
>   f = let v = unsafePerformIO (newIORef []) in \x -> v
> 
> But if you have
> 
>   f x = unsafePerformIO (newIORef [x])
> 
> it's ok because the x on the right hand side makes that transformation 
> invalid.

Okay, thanks, I think I get it now. Still, quite subtle. I wouldn't want
to apply that kind of reasoning to code I write if it can be helped.
Concentrating global mutable variables in Darcs.Util.Global may have
been a good idea to start with, so we can just disable all possibly
disastrous optimizations in that one module. However it seems this
hasn't really worked out to contain use of unsafePerformIO:

> grep -rwl unsafePerformIO src
src/Darcs/Patch/Match.hs
src/Darcs/Patch/Info.hs
src/Darcs/Patch/Progress.hs
src/Darcs/Patch/Prim/Named.hs
src/Darcs/Util/IsoDate.hs
src/Darcs/Util/Global.hs
src/Darcs/Util/AtExit.hs
src/Darcs/Util/Download.hs
src/Darcs/Util/Printer/Color.hs
src/Darcs/Util/ByteString.hs
src/Darcs/Util/Ssh.hs
src/Darcs/Util/Progress.hs
src/Darcs/Repository/Hashed.hs
src/Darcs/Repository/Cache.hs
src/Darcs/UI/Email.hs
msg21720 (view) Author: ganesh Date: 2020-01-25.22:23:52
Updated version with more explanation. I'm going to screen this now as
I think it makes sense while we still have the unsafePerformIO.

1 patch for repository darcs-unstable@darcs.net:screened:

patch 6fccddb0ab22715c8ff345f4032303941cfd647d
Author: Ganesh Sittampalam <ganesh@earth.li>
Date:   Sat Jan 25 17:22:00 GMT 2020
  * add -fno-cse to D.P.Prim.Named given the unsafePerformIO
Attachments
msg21747 (view) Author: bfrk Date: 2020-01-27.15:42:46
> I think it makes sense while we still have the unsafePerformIO.

Yup.

________________________________

Helmholtz-Zentrum Berlin für Materialien und Energie GmbH

Mitglied der Hermann von Helmholtz-Gemeinschaft Deutscher Forschungszentren e.V.

Aufsichtsrat: Vorsitzender Dr. Volkmar Dietz, stv. Vorsitzende Dr. Jutta Koch-Unterseher
Geschäftsführung: Prof. Dr. Bernd Rech (Sprecher), Prof. Dr. Jan Lüning, Thomas Frederking

Sitz Berlin, AG Charlottenburg, 89 HRB 5583

Postadresse:
Hahn-Meitner-Platz 1
D-14109 Berlin
msg21772 (view) Author: bfrk Date: 2020-01-30.18:32:24
Looks good.
msg21773 (view) Author: bfrk Date: 2020-01-30.18:39:52
Looks good.
History
Date User Action Args
2019-09-22 22:33:29ganeshcreate
2019-09-22 22:34:36ganeshsetstatus: needs-screening -> in-discussion
2019-09-23 12:14:09bfrksetmessages: + msg21546
2019-09-23 13:06:34ganeshsetmessages: + msg21550
2019-09-23 16:38:38bfrksetmessages: + msg21553
2019-09-23 16:52:30ganeshsetmessages: + msg21554
2019-09-23 21:55:14bfrksetmessages: + msg21563
2020-01-25 22:23:52ganeshsetfiles: + patch-preview.txt, add-_fno_cse-to-d_p_prim_named-given-the-unsafeperformio.dpatch, unnamed
messages: + msg21720
2020-01-25 22:28:29ganeshsetstatus: in-discussion -> needs-review
2020-01-27 15:42:47bfrksetmessages: + msg21747
2020-01-30 18:32:24bfrksetstatus: needs-review -> accepted
messages: + msg21772
2020-01-30 18:39:52bfrksetmessages: + msg21773