Factor out readField (#14364)
ClosedPublic

Authored by tdammers on Oct 19 2017, 2:33 AM.

Details

Summary

Improves compiler performance of deriving Read instances, as suggested in the issue.

Additionally, we introduce readSymField, a companion to readField that parses symbol-type fields (where the field name is a symbol, e.g. (#), rather than an alphanumeric identifier. The decision between these two functions is made a compile time, because we already know which one we need based on the field name.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
tdammers created this revision.Oct 19 2017, 2:33 AM
tdammers edited the summary of this revision. (Show Details)Oct 19 2017, 3:03 AM
bgamari accepted this revision.Oct 19 2017, 8:06 AM

Yep.

This revision is now accepted and ready to land.Oct 19 2017, 8:06 AM
RyanGlScott requested changes to this revision.Oct 19 2017, 8:13 AM
RyanGlScott added a subscriber: RyanGlScott.

This transformation doesn't preserve the original behavior of deriving Read, as the Harbormaster test failure for drvrun012 shows. The program in drvrun012 is:

-- Tests readings of record syntax

module Main where

data Foo = Foo { x :: Baz, y :: Maybe Int } deriving (Read,Show)

infix 0 :%%
data Baz = Int :%% Int deriving( Read,Show)


main = print (read "Foo { x = 1 :%% 2, y = Just 4 }" :: Foo)

Before this change, this would emit the following Read instance for Foo:

instance Read Foo where                                                            
  readPrec                                                                              
    = parens                                                                            
        (prec                                                    
           11                                                                                    
           (do expectP (Ident "Foo")                                      
               expectP (Punc "{")                                         
               expectP (Ident "x")                                        
               expectP (Punc "=")
               a1 <- reset readPrec
               expectP (Punc ",")
               expectP (Ident "y")
               expectP (Punc "=")
               a2 <- reset readPrec
               expectP (Punc "}")
               return (Foo a1 a2)))

After this change, it now emits:

instance Read Foo where
  readPrec
    = parens
        (prec
           11
           (do expectP (Ident "Foo")
               expectP (Punc "{")
               a1 <- readField "x" readPrec
               expectP (Punc ",")
               a2 <- readField "y" readPrec
               expectP (Punc "}")
               GHC.Base.return (Foo a1 a2)))

readField :: String -> ReadPrec a -> ReadPrec a
readField fieldName readVal = do
        expectP (L.Ident fieldName)
        expectP (L.Punc "=")
        readVal

Notice that we're not calling reset anymore! This change should be sufficient to fix the issue, though:

diff --git a/compiler/typecheck/TcGenDeriv.hs b/compiler/typecheck/TcGenDeriv.hs                   
index 2d004be..26ac853 100644                                                                      
--- a/compiler/typecheck/TcGenDeriv.hs                                                             
+++ b/compiler/typecheck/TcGenDeriv.hs                                                             
@@ -1079,7 +1079,9 @@ gen_Read_binds get_fixity loc tycon                                          
             (nlVarPat a)                                                                          
             (nlHsApps                                                                             
               read_field                                                                          
-              [nlHsLit (mkHsString lbl_str), nlHsVar readPrec_RDR]                                
+              [ nlHsLit (mkHsString lbl_str)                                                      
+              , nlHsVarApps reset_RDR [readPrec_RDR]                                              
+              ]                                                                                   
             )                                                                                     
           )                                                                                       
         ]
compiler/typecheck/TcGenDeriv.hs
889–905

deriving Read now emits different code, so please update this example accordingly.

libraries/base/GHC/Read.hs
364–378

It would be helpful to add some Haddock comments for readField and readSymField explaining what each function does. Perhaps in a separate (non-Haddock) comment, also explain that these were introduced to resolve Trac #14364.

This revision now requires changes to proceed.Oct 19 2017, 8:13 AM
tdammers updated this revision to Diff 14410.Oct 19 2017, 9:34 AM
  • Fixed regression: derived Read instance must retain reset call
  • Document readField / readSymField
tdammers updated this revision to Diff 14411.Oct 19 2017, 9:50 AM
  • Factor out readField (Trac #14364)
  • Fixed regression: derived Read instance must retain reset call
  • Document readField / readSymField
  • Revert "Make layLeft and reduceDoc stricter (Trac #7258)"

Sorry for the line noise, haven't quite mastered the art of wrangling arcanist yet.

RyanGlScott added inline comments.Oct 19 2017, 1:54 PM
compiler/nativeGen/RegAlloc/Linear/Main.hs
812–822 ↗(On Diff #14411)

I don't think you intended to check in these changes?

compiler/typecheck/TcGenDeriv.hs
889–905

Don't forget about this!

tdammers updated this revision to Diff 14418.Oct 19 2017, 2:17 PM

Removed the linear alloc optimization commit that had slipped in.

tdammers updated this revision to Diff 14419.Oct 19 2017, 2:20 PM
tdammers marked 4 inline comments as done.Oct 19 2017, 2:22 PM
tdammers added inline comments.
compiler/nativeGen/RegAlloc/Linear/Main.hs
812–822 ↗(On Diff #14411)

You are right, that was me fat-fingering arcanist; these belong in a different patch. Latest rev. should be fine.

RyanGlScott added inline comments.Oct 19 2017, 2:34 PM
compiler/typecheck/TcGenDeriv.hs
903

I think this should be x <- readField "f1" (reset readPrec)?

tdammers updated this revision to Diff 14449.Oct 24 2017, 7:35 AM
tdammers marked an inline comment as done.

Fixed stat bounds for test case

tdammers updated this revision to Diff 14450.Oct 24 2017, 7:36 AM

Fixed perf stat tests

tdammers updated this revision to Diff 14451.Oct 24 2017, 7:41 AM
tdammers marked an inline comment as done.

Corrected readField example (D4108)

bgamari accepted this revision.Oct 25 2017, 1:24 PM

Thanks for the careful reviews, @RyanGlScott!

Otherwise this looks good to me.

Note that this may not be the end of the story: future work includes potentially refactoring these instances to instead use Applicative instead of Monad..

This revision was automatically updated to reflect the committed changes.