Avoids redundant substitution in unifyTypes when possible by mikesol · Pull Request #4163 · purescript/purescript

mikesol · 2021-07-17T19:23:27Z

Description of the change

This change speeds up PureScript's compilation for programs that make heavy use of typeclasses. It is unclear the effect it has (if any) on the compilation of other programs.

Benchmarks

Compiling Loops.purs on my ubuntu machine, I get the following result without this change:

$ time npx spago -x examples.dhall build
Compiling WAGS.Lib.Example.Loops
Warning 1 of 2:
  in module WAGS.Lib.Example.Loops
  at examples/loops/Loops.purs:73:1 - 74:165 (line 73, column 1 - line 74, column 165)
    The inferred kind for the type declaration Instruments'' contains polymorphic kinds.
    Consider adding a top-level kind signature as a form of documentation.
      type Instruments'' :: forall k. k -> Row k -> Row k
  in type synonym Instruments''
  See https://github.com/purescript/documentation/blob/master/errors/MissingKindDeclaration.md for more information,
  or to contribute content related to this warning.
Warning 2 of 2:
  in module WAGS.Lib.Example.Loops
  at examples/loops/Loops.purs:76:1 - 77:23 (line 76, column 1 - line 77, column 23)
    The inferred kind for the type declaration Instruments' contains polymorphic kinds.
    Consider adding a top-level kind signature as a form of documentation.
      type Instruments' :: forall k. k -> Row k
  in type synonym Instruments'
  See https://github.com/purescript/documentation/blob/master/errors/MissingKindDeclaration.md for more information,
  or to contribute content related to this warning.
[info] Build succeeded.
real    6m42,643s
user    51m26,103s
sys     11m33,138s

With this change, I get the following result:

07:49 meeshkan-abel@Abel:~/mike/purescript-wags-lib$ time npx spago -x examples.dhall build
Compiling WAGS.Lib.Example.Loops
Warning 1 of 5:

  in module WAGS.Lib.Example.Loops
  at examples/loops/Loops.purs:117:12 - 117:13 (line 117, column 12 - line 117, column 13)

    Name b was introduced but not used.

  in value declaration append

  See https://github.com/purescript/documentation/blob/master/errors/UnusedName.md for more information,
  or to contribute content related to this warning.

Warning 2 of 5:

  in module WAGS.Lib.Example.Loops
  at examples/loops/Loops.purs:240:37 - 240:41 (line 240, column 37 - line 240, column 41)

    Name time was introduced but not used.

  in value declaration piece

  See https://github.com/purescript/documentation/blob/master/errors/UnusedName.md for more information,
  or to contribute content related to this warning.

Warning 3 of 5:

  in module WAGS.Lib.Example.Loops
  at examples/loops/Loops.purs:308:8 - 308:13 (line 308, column 8 - line 308, column 13)

    Name state was introduced but not used.

  in value declaration render

  See https://github.com/purescript/documentation/blob/master/errors/UnusedName.md for more information,
  or to contribute content related to this warning.

Warning 4 of 5:

  in module WAGS.Lib.Example.Loops
  at examples/loops/Loops.purs:73:1 - 74:165 (line 73, column 1 - line 74, column 165)

    The inferred kind for the type declaration Instruments'' contains polymorphic kinds.
    Consider adding a top-level kind signature as a form of documentation.
                                                         
      type Instruments'' :: forall k. k -> Row k -> Row k
                                                         

  in type synonym Instruments''

  See https://github.com/purescript/documentation/blob/master/errors/MissingKindDeclaration.md for more information,
  or to contribute content related to this warning.

Warning 5 of 5:

  in module WAGS.Lib.Example.Loops
  at examples/loops/Loops.purs:76:1 - 77:23 (line 76, column 1 - line 77, column 23)

    The inferred kind for the type declaration Instruments' contains polymorphic kinds.
    Consider adding a top-level kind signature as a form of documentation.
                                               
      type Instruments' :: forall k. k -> Row k
                                               

  in type synonym Instruments'

  See https://github.com/purescript/documentation/blob/master/errors/MissingKindDeclaration.md for more information,
  or to contribute content related to this warning.


[info] Build succeeded.

real    3m27,583s
user    26m43,771s
sys     4m34,487s

These times are pretty consistent across several tests. In general, there is a 1.5-2x speedup for heavily-constrained files when using a purs built with this PR.

Checklist:

Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
Added myself to CONTRIBUTORS.md (if this is my first contribution)
Linked any existing issues or proposals that this pull request should close (there are none)
Updated or added relevant documentation (added a comment)
Added a test for the contribution (not applicable)

mikesol · 2021-07-19T05:55:28Z

@natefaubion if you have the chance, could you add a note to the PR that describes your initial proposal about skipping checkSubstitution in certain circumstances? I'd like to write better documentation about why this works and under what conditions the skipping can happen. It would be even better if the type system could reinforce that.

hdgarrood · 2021-07-19T21:48:52Z

I'm not keen on this if it's going to make the compiler diagnostics worse. An error message which said that there was a failure to unify an unknown type with some other type is most likely just wrong; unknowns should unify with everything, shouldn't they?

hdgarrood · 2021-07-19T21:53:14Z

In the other golden example too - it's more useful to be told that Record didn't unify with Function Int rather than just Function t2; if the compiler knows that it's dealing with a function which takes an Int, I expect it to tell me that in the error message, in case that helps me track down the error. I think in particular it shouldn't claim that the type is unknown when it is actually known.

mikesol · 2021-07-20T03:33:35Z

@hdgarrood Thanks for the feedback!
I made a couple changes that I believe resolve this issue. I also think the changes have led to slight readability improvements in both of the .out files that were modified.

CHANGELOG.d/fix_avoids redundant substitution.md

hdgarrood · 2021-07-20T15:35:02Z

Yep, it does indeed look like your most recent change has resolved that issue - thanks! I'd consider both of those changes to be improvements.

natefaubion · 2021-07-20T16:08:13Z

@natefaubion if you have the chance, could you add a note to the PR that describes your initial proposal about skipping checkSubstitution in certain circumstances? I'd like to write better documentation about why this works and under what conditions the skipping can happen. It would be even better if the type system could reinforce that.

The invariant to preserve is that we should never unify/solve against an unknown if there is already a solution for that unification variable. Or to put it another way, we must always unify against a known solution if one exists. Unification is non-local, so anytime we solve something, this potentially has an effect elsewhere. The coarse way of guarding against this is to always apply the current substitution whenever unifying two types, but this results in more rewrites than necessary. Since substitution only affects unification variables, it is enough to only perform this check and expansion on demand when we go to solve a unification variable. We can check if that unification variable has a solution, and if it does, continue to unify against it's solution rather than solving.

In practice there are likely other places to apply a substitution, for example, you'll probably want to apply the substitution against the solution for a unification variable.

JordanMartinez · 2021-07-23T14:26:28Z

I noticed CI was still waiting for us to approve this run. So, I fixed that.

mikesol · 2021-07-26T08:16:18Z

I noticed CI was still waiting for us to approve this run. So, I fixed that.

I unfortunately cannot reproduce the most recent CI failure on my local build:

purescript> graph       
purescript>   should match the graph fixture [✔]
purescript>   should fail when trying to include non-existing modules in the graph [✔]
purescript>             
purescript> Finished in 119.4557 seconds
purescript> 1116 examples, 0 failures
purescript> Test suite tests passed
Completed 2 action(s).  
10:58 meeshkan-abel@Abel:~/mike/purescript$ git commit -a
On branch faster
Your branch is up to date with 'origin/faster'.

Does anyone have tips on reproducing the test error locally?

JordanMartinez · 2021-07-26T14:24:05Z

I've checked this out and will be testing it locally to see what's going on.

JordanMartinez · 2021-07-26T14:26:28Z

For reference, CI fails with this error:

Failures:
  purescript    > 
  purescript    >   tests/TestCompiler.hs:175:7: 
  purescript    >   1) compiler, Failing examples, 'InstanceSigsIncorrectType.purs' should fail to compile
  purescript    >        Test output differed from 'tests/purs/failing/InstanceSigsIncorrectType.out'; got:
  purescript    >        Error found:
  purescript    >        in module Main
  purescript    >        at tests/purs/failing/InstanceSigsIncorrectType.purs:8:1 - 10:13 (line 8, column 1 - line 10, column 13)
  purescript    >        
  purescript    >          Could not match type
  purescript    >                   
  purescript    >            Boolean
  purescript    >                   
  purescript    >          with type
  purescript    >                  
  purescript    >            Number
  purescript    >                  
  purescript    >        
  purescript    >        while trying to match type Foo$Dict Boolean
  purescript    >          with type Foo$Dict Number
  purescript    >        while checking that expression Foo$Dict { foo: true
  purescript    >                                                }          
  purescript    >          has type Foo$Dict Number
  purescript    >        in value declaration fooNumber
  purescript    >        
  purescript    >        See https://github.com/purescript/documentation/blob/master/errors/TypesDoNotUnify.md for more information,
  purescript    >        or to contribute content related to this error.
  purescript    >        
  purescript    > 
  purescript    >   To rerun use: --match "/compiler/Failing examples/'InstanceSigsIncorrectType.purs' should fail to compile/"
  purescript    > 
  purescript    > Randomized with seed 1897078963
  purescript    > 
  purescript    > Finished in 272.5232 seconds
  purescript    > 1098 examples, 1 failure
  purescript    > Test suite tests failed

JordanMartinez · 2021-07-26T19:03:05Z

When I ran stack test --fast locally, I got this output. Looks like the snapshot tests need to be updated (assuming these updates are valid):

Failures:   
purescript>             
purescript>   tests/TestCompiler.hs:175:7: 
purescript>   1) compiler, Failing examples, '3765.purs' should fail to compile
purescript>        Test output differed from 'tests/purs/failing/3765.out'; got:
purescript>        Error found:
purescript>        in module Main
purescript>        at tests/purs/failing/3765.purs:6:23 - 6:24 (line 6, column 23 - line 6, column 24)
purescript>             
purescript>          Could not match type
purescript>                      
purescript>            ( b :: Int
purescript>            ...       
purescript>            | t0      
purescript>            )         
purescript>                      
purescript>          with type
purescript>                      
purescript>            ( a :: Int
purescript>            ...       
purescript>            | t0      
purescript>            )         
purescript>                      
purescript>             
purescript>        while trying to match type { b :: Int
purescript>                                   | t0      
purescript>                                   }         
purescript>          with type { a :: Int
purescript>                    | t0      
purescript>                    }         
purescript>        while checking that expression x
purescript>          has type { b :: Int
purescript>                   | t0      
purescript>                   }         
purescript>        in value declaration mkTricky
purescript>             
purescript>        where t0 is an unknown type
purescript>             
purescript>        See https://github.com/purescript/documentation/blob/master/errors/TypesDoNotUnify.md for more information,
purescript>        or to contribute content related to this error.
purescript>             
purescript>             
purescript>   To rerun use: --match "/compiler/Failing examples/'3765.purs' should fail to compile/"
purescript>             
purescript>   tests/TestCompiler.hs:175:7: 
purescript>   2) compiler, Failing examples, 'DuplicateProperties.purs' should fail to compile
purescript>        Test output differed from 'tests/purs/failing/DuplicateProperties.out'; got:
purescript>        Error found:
purescript>        in module DuplicateProperties
purescript>        at tests/purs/failing/DuplicateProperties.purs:12:18 - 12:32 (line 12, column 18 - line 12, column 32)
purescript>             
purescript>          Could not match type
purescript>                       
purescript>            ( y :: Unit
purescript>            ...        
purescript>            )          
purescript>                       
purescript>          with type
purescript>                       
purescript>            ( x :: Unit
purescript>            ...        
purescript>            | t0       
purescript>            )          
purescript>                       
purescript>             
purescript>        while trying to match type Test         
purescript>                                     ( y :: Unit
purescript>                                     )          
purescript>          with type Test         
purescript>                      ( x :: Unit
purescript>                      | t0       
purescript>                      )          
purescript>        while checking that expression subtractX hasX
purescript>          has type Test         
purescript>                     ( x :: Unit
purescript>                     | t0       
purescript>                     )          
purescript>        in value declaration baz
purescript>             
purescript>        where t0 is an unknown type
purescript>             
purescript>        See https://github.com/purescript/documentation/blob/master/errors/TypesDoNotUnify.md for more information,
purescript>        or to contribute content related to this error.
purescript>             
purescript>             
purescript>   To rerun use: --match "/compiler/Failing examples/'DuplicateProperties.purs' should fail to compile/"
purescript>             
purescript>   tests/TestCompiler.hs:175:7: 
purescript>   3) compiler, Failing examples, 'InstanceSigsIncorrectType.purs' should fail to compile
purescript>        Test output differed from 'tests/purs/failing/InstanceSigsIncorrectType.out'; got:
purescript>        Error found:
purescript>        in module Main
purescript>        at tests/purs/failing/InstanceSigsIncorrectType.purs:8:1 - 10:13 (line 8, column 1 - line 10, column 13)
purescript>             
purescript>          Could not match type
purescript>                   
purescript>            Boolean
purescript>                   
purescript>          with type
purescript>                  
purescript>            Number
purescript>                  
purescript>             
purescript>        while trying to match type Foo$Dict Boolean
purescript>          with type Foo$Dict Number
purescript>        while checking that expression Foo$Dict { foo: true
purescript>                                                }          
purescript>          has type Foo$Dict Number
purescript>        in value declaration fooNumber
purescript>             
purescript>        See https://github.com/purescript/documentation/blob/master/errors/TypesDoNotUnify.md for more information,
purescript>        or to contribute content related to this error.
purescript>             
purescript>             
purescript>   To rerun use: --match "/compiler/Failing examples/'InstanceSigsIncorrectType.purs' should fail to compile/"
purescript>             
purescript> Randomized with seed 1321292021
purescript>             
purescript> Finished in 391.0848 seconds
purescript> 1123 examples, 3 failures
purescript> Test suite tests failed
Completed 2 action(s).  
Test suite failure for package purescript-0.14.3
    tests:  exited with: ExitFailure 1
Logs printed to console

mikesol · 2021-07-27T06:06:31Z

After pulling in the latest changes from master the issue is now that there is an infinite hang when running the tests:

purescript>     '2663.purs' should compile and run without error [✔] (82ms)
purescript>     '2689.purs' should compile and run without error [✔] (175ms)
purescript>     '2756.purs' should compile and run without error [✔] (114ms)
purescript>     '2787.purs' should compile and run without error [✔] (111ms)
purescript>     '2795.purs' should compile and run without error [✔] (72ms)
purescript>     '2803.purs' should compile and run without error [✔] (140ms)
purescript>     '2806.purs' should compile and run without error [✔] (89ms)
purescript>     '2947.purs' should compile and run without error [✘] (59ms)
purescript>     '2958.purs' should compile and run without error [✔] (132ms)
purescript>     '2972.purs' should compile and run without error [✘] (39ms)
Progress 1/2: purescript^C

I control-C'd after leaving it on for a few hours. My guess is that something that went into the codebase recently doesn't play nice with this PR - I'll have to dig to find out more.

rhendric · 2021-07-27T15:18:03Z

Temporarily merging #4126 into your branch might help you diagnose that hang, FYI.

mikesol · 2021-08-07T19:47:28Z

@rhendric your patch works like a charm! I can fish out the errors now: there are many (45 in total) and they're all of the form:

purescript>        Error found:
purescript>        in module Main
purescript>        at tests/purs/passing/Superclasses3.purs:36:1 - 36:37 (line 36, column 1 - line 36, column 37)
purescript>        
purescript>          Unknown data constructor Control.Monad.Monad$Dict
purescript>        
purescript>        while inferring the type of Monad$Dict
purescript>        in value declaration monadMTrace

or

purescript>        Error found:
purescript>        in module Main
purescript>        at tests/purs/passing/RebindableSyntax.purs:25:1 - 26:28 (line 25, column 1 - line 26, column 28)
purescript>        
purescript>          Unknown data constructor Data.Functor.Functor$Dict
purescript>        
purescript>        while inferring the type of Functor$Dict
purescript>        in value declaration functorConst
purescript>        
purescript>        See https://github.com/purescript/documentation/blob/master/errors/UnknownName.md for more information,
purescript>        or to contribute content related to this error.

Interestingly, this error wasn't present when I started working on the PR. Does it look familiar to anyone?

rhendric · 2021-08-07T19:59:16Z

Those $Dict constructors the messages are complaining about are the constructors I introduced in #4125, and it looks like you started this PR just before that one was merged, which might explain it. On the other hand, those errors probably wouldn't have caused a hang; are there also internal error stack traces elsewhere in your test output? Those might be the real issue.

mikesol · 2021-08-08T05:11:15Z

Those $Dict constructors the messages are complaining about are the constructors I introduced in #4125, and it looks like you started this PR just before that one was merged, which might explain it. On the other hand, those errors probably wouldn't have caused a hang; are there also internal error stack traces elsewhere in your test output? Those might be the real issue.

The only stack trace I could find in the logs (which appeared 4 times) is:

purescript>   src/Language/PureScript/Environment.hs:576:14: 
purescript>   42) bundle, Bundle examples, 'ObjectShorthand.purs' should compile, bundle and run without error
purescript>        uncaught exception: ErrorCall
purescript>        An internal error occurred during compilation: Data constructor not found
purescript>        Please report this at https://github.com/purescript/purescript/issues
purescript>        CallStack (from HasCallStack):
purescript>          error, called at src/Language/PureScript/Crash.hs:23:3 in purescript-cst-0.3.0.0-1JSI7w0XXsV3fsrGnkQELx:Language.PureScript.Crash
purescript>          internalError, called at src/Language/PureScript/Environment.hs:576:14 in purescript-cst-0.3.0.0-1JSI7w0XXsV3fsrGnkQELx:Language.PureScript.Environment

When navigating to line 576 of Environment.hs, this turns out to be the same "Data constructor not found" error.

While this PR doesn't interact explicitly with any of the things removed in #4125 (ie it never touched TypeClassDictionaryConstructorApp), there could have been something subtle going on that I failed to account for. From my initial reading, the first port of call would be solveType. As I'm delaying calls to solveType, checkSubstitution has less information, which may adversely effect the Environment in checkEnv. Then, when corefn desugaring happens in Make.hs, this informatoin is not available.

This is all hand-wavey intuition though, I'd need to really dig in to confirm what's going on.

mikesol · 2021-11-29T14:35:14Z

Closing as it is too challenging for me to work on this atm - I need to understand the type checker better first.

Mike Solomon added 2 commits July 17, 2021 22:22

Avoids redundant substitution in unifyTypes when possible

2cd7e4c

Modifies out files

60b5597

mikesol force-pushed the faster branch from b1e0868 to 60b5597 Compare July 18, 2021 06:24

Massages output

a175d46

rhendric mentioned this pull request Jul 19, 2021

Improve row type error messages #4159

Merged

3 tasks

Mike Solomon added 4 commits July 19, 2021 07:20

Changes DuplicateProperties.purs

d226105

Adds name to contributor list

e33e5b7

Adds line to changelog

061a9a6

Adds TODO comment

6c4ed85

mikesol marked this pull request as ready for review July 19, 2021 05:16

Mike Solomon added 2 commits July 20, 2021 06:17

Restores better error messages

facdb8e

Improves error messages

13584d6

Changes name of function

321bfc2

JordanMartinez reviewed Jul 20, 2021

View reviewed changes

CHANGELOG.d/fix_avoids redundant substitution.md Outdated Show resolved Hide resolved

Mike Solomon added 2 commits July 20, 2021 22:10

Adds comment for unify types

6981f91

Jordan's suggestion

9c82f26

mikesol closed this Nov 29, 2021

Conversation

mikesol commented Jul 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikesol commented Jul 19, 2021

Uh oh!

hdgarrood commented Jul 19, 2021

Uh oh!

hdgarrood commented Jul 19, 2021

Uh oh!

mikesol commented Jul 20, 2021

Uh oh!

Uh oh!

hdgarrood commented Jul 20, 2021

Uh oh!

natefaubion commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JordanMartinez commented Jul 23, 2021

Uh oh!

mikesol commented Jul 26, 2021

Uh oh!

JordanMartinez commented Jul 26, 2021

Uh oh!

JordanMartinez commented Jul 26, 2021

Uh oh!

JordanMartinez commented Jul 26, 2021

Uh oh!

mikesol commented Jul 27, 2021

Uh oh!

rhendric commented Jul 27, 2021

Uh oh!

mikesol commented Aug 7, 2021

Uh oh!

rhendric commented Aug 7, 2021

Uh oh!

mikesol commented Aug 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikesol commented Nov 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mikesol commented Jul 17, 2021 •

edited

Loading

natefaubion commented Jul 20, 2021 •

edited

Loading

mikesol commented Aug 8, 2021 •

edited

Loading