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

Inspection tests for Prisms #28

Closed
chshersh opened this issue Oct 11, 2020 · 9 comments · Fixed by #48
Closed

Inspection tests for Prisms #28

chshersh opened this issue Oct 11, 2020 · 9 comments · Fixed by #48
Assignees
Labels
hacktoberfest help wanted Extra attention is needed tests Testing, property testing, DocTests

Comments

@chshersh
Copy link
Contributor

No description provided.

@chshersh chshersh added the tests Testing, property testing, DocTests label Oct 11, 2020
@chshersh chshersh self-assigned this Oct 13, 2020
@chshersh
Copy link
Contributor Author

Tests are implemented, but they are failing after initial performance improvements. Current GHC Core result:

test/Test/Prolens/Inspection.hs:89:1: matchMarkPrism === matchMarkManual failed:
    LHS:
        matchMarkPrism :: Grade -> Maybe Int
        [LclIdX,
         Arity=1,
         Str=<S,1*U>,
         Unf=Unf{Src=InlineStable, TopLvl=True, Value=True, ConLike=True,
                 WorkFree=True, Expandable=True,
                 Guidance=ALWAYS_IF(arity=0,unsat_ok=True,boring_ok=True)
                 Tmpl= matchMarkPrism_s6zt
                       `cast` (<Grade>_R ->_R N:First[0] <Int>_N
                               :: Coercible (Grade -> First Int) (Grade -> Maybe Int))}]
        matchMarkPrism
          = matchMarkPrism_s6zt
            `cast` (<Grade>_R ->_R N:First[0] <Int>_N
                    :: Coercible (Grade -> First Int) (Grade -> Maybe Int))
        
        matchMarkPrism_s6zt :: Grade -> First Int
        [LclId,
         Arity=1,
         Str=<S,1*U>,
         Unf=Unf{Src=InlineStable, TopLvl=True, Value=True, ConLike=True,
                 WorkFree=True, Expandable=True,
                 Guidance=ALWAYS_IF(arity=1,unsat_ok=True,boring_ok=False)
                 Tmpl= \ (x [Occ=Once!] :: Grade) ->
                         case x of {
                           Mark a [Occ=Once] ->
                             (Just @ Int a)
                             `cast` (Nth:3 (<Int>_R ->_R Sym (N:First[0]) <Int>_N)
                                     :: Coercible (Maybe Int) (First Int));
                           Other _ [Occ=Dead] ->
                             (Nothing @ Int)
                             `cast` (Sym (N:First[0]) <Int>_N
                                     :: Coercible (Maybe Int) (First Int))
                         }}]
        matchMarkPrism_s6zt
          = \ (x [Dmd=<S,1*U>] :: Grade) ->
              case x of {
                Mark a ->
                  (Just @ Int a)
                  `cast` (Nth:3 (<Int>_R ->_R Sym (N:First[0]) <Int>_N)
                          :: Coercible (Maybe Int) (First Int));
                Other ipv [Dmd=<L,A>] ->
                  (Nothing @ Int)
                  `cast` (Sym (N:First[0]) <Int>_N
                          :: Coercible (Maybe Int) (First Int))
              }
        
    RHS:
        matchMarkManual :: Grade -> Maybe Int
        [LclIdX,
         Arity=1,
         Str=<S,1*U>,
         Unf=Unf{Src=InlineStable, TopLvl=True, Value=True, ConLike=True,
                 WorkFree=True, Expandable=True,
                 Guidance=ALWAYS_IF(arity=1,unsat_ok=True,boring_ok=False)
                 Tmpl= \ (grade [Occ=Once!] :: Grade) ->
                         case grade of {
                           Mark n [Occ=Once] -> Just @ Int n;
                           Other _ [Occ=Dead] -> Nothing @ Int
                         }}]
        matchMarkManual
          = \ (grade [Dmd=<S,1*U>] :: Grade) ->
              case grade of {
                Mark n -> Just @ Int n;
                Other ipv [Dmd=<L,A>] -> Nothing @ Int
              }

@chshersh chshersh added the help wanted Extra attention is needed label Oct 13, 2020
@chshersh chshersh removed their assignment Oct 13, 2020
@CristhianMotoche
Copy link
Contributor

Hi @chshersh I'm also getting an error when I run the tests Performance Inspection Testing for Lens:

  test/Test/Prolens/Inspection.hs:52:13:
  1) Performance Inspection Testing, Lens, set, setting single via lens is efficient as manual record update
       predicate failed on: Failure "test/Test/Prolens/Inspection.hs:52:15: ...        "

The expanded error message is the following:

setNameViaLens === setNameManually failed:
    LHS:
        setNameViaLens :: Haskeller -> Haskeller
        [LclIdX,
         Unf=Unf{Src=<vanilla>, TopLvl=True, Value=False, ConLike=False,
                 WorkFree=False, Expandable=False, Guidance=IF_ARGS [] 110 0}]
        setNameViaLens
          = set
              @ (->)
              @ Haskeller
              @ Haskeller
              @ String
              @ String
              (Eq# @ (* -> * -> *) @ (->) @ (->) @~ (<(->)>_N :: (->) ~ (->)))
              (nameL @ (->) $fStrong->)
              (unpackCString# "Dmitrii"#)

    RHS:
        setNameManually :: Haskeller -> Haskeller
        [LclIdX,
         Arity=1,
         Unf=Unf{Src=<vanilla>, TopLvl=True, Value=True, ConLike=True,
                 WorkFree=True, Expandable=True, Guidance=IF_ARGS [20] 70 40}]
        setNameManually
          = \ (h :: Haskeller) ->
              case h of { Haskeller ds_dcjT ds_dcjU ds_dcjV ->
              Haskeller (unpackCString# "Dmitrii"#) ds_dcjU ds_dcjV
              }

I think I'm not enabling a flag or something similar. Please, let me know and I could give it a try.

@chshersh
Copy link
Contributor Author

Hi @CristhianMotoche! I think you build the project without optimizations. E.g. by passing the -O0 option in Cabal or --fast in Stack. From the core output I see that the definition of setNameViaLens looks almost exactly as the Haskell code. So it means that no inlining during optimizations was applied at all, so, most likely, there were no optimizations performed.

By default, Cabal passes -O1 optimization flag, so this should be enough for tests to pass. But you can try running them manually:

cabal clean
cabal test all -O2 --enable-tests

@CristhianMotoche
Copy link
Contributor

Thanks @chshersh I enabled --fast. I'll take a look at the issue now.

@CristhianMotoche
Copy link
Contributor

I've never done inspection testing before, so it could take me some time to address this issue.
If anyone else wants to take a look at the issue. Go ahead!

@CristhianMotoche
Copy link
Contributor

Hi! I'm continue learning about inspection testing.

The first thing that I tried was to see less Core so I added a few GHC flags as suggested in the inspection-testing library and the output looks like this:

matchMarkPrism === matchMarkManual failed:
    LHS:
        matchMarkPrism = matchMarkPrism `cast` <Co:4>

        matchMarkPrism
          = \ x ->
              case x of {
                Mark a -> (Just a) `cast` <Co:6>;
                Other ipv -> Nothing `cast` <Co:3>
              }

    RHS:
        matchMarkManual
          = \ grade ->
              case grade of {
                Mark n -> Just n;
                Other ipv -> Nothing
              }

They look almost the same except by this cast ... in some lines, which I think is introduced due to coerce. I'm not exactly sure what would be my next steps, so, please, let me know if you have any suggestions.

Thanks in advance.

@chshersh
Copy link
Contributor Author

@CristhianMotoche Great work! 👏 Yes, that's I saw as well. The implementations in Core are almost identical. So the performance of both implementations should be the same. AFAIK, Haskell implements zero-cost coercions, so those two functions must be the same in terms of performance. But, because the look differently in Core, the test with inspection-testing library fails 😞 Maybe GHC doesn't optimize this enough to remove this final cast?

I would love to have an answer to that! Possible solutions: fix the inspection-testing library to consider such cases equal or fix GHC to optimize this finally. Or maybe change our implementation of prisms to something more efficient (but we've already squeezed everything we could from it).

To move this forward, I can recommend the following steps:

  • Ask GHC developers on GHC IRC channels about what's happening. I believe, they are more experienced in Core and probably can recommend something
  • Open an issue in GHC regarding optimization improvements
  • Open an issue in the inspection-testing library to discuss how to resolve this

@vrom911 vrom911 self-assigned this Nov 4, 2020
vrom911 added a commit that referenced this issue Nov 4, 2020
@vrom911
Copy link
Member

vrom911 commented Nov 4, 2020

@CristhianMotoche I managed to make it work!
Apparently, nested newtypes didn't inline well.
I tried a few options: using oneShot and inline, but didn't help much, as well as custom First newtype with inlined instances.
But to inline First (aka Maybe) into the Forget itself seem to help!

chshersh pushed a commit that referenced this issue Nov 4, 2020
* [#28] Inspection tests for Prisms

Resolves #28

* Remove _Mark inline
@CristhianMotoche
Copy link
Contributor

I'm glad to know you solved it @vrom911 Good job! 🎉 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest help wanted Extra attention is needed tests Testing, property testing, DocTests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants