Thanks!
I've restarted the Travis builds that timed out.

Nice result!

I'm wondering if it's be worth factoring this into the derived compare composed with a function which strips source spans. It'd mean two traversals, but you'd get a function which would probably be useful elsewhere (for similar optimizations or other things). If the generic annotations work gets merged later, it'd literally become an fmap or something similar.

Also please don't forget to update contributors.md.

I've added myself to CONTRIBUTORS.md, thank you!

I'm wondering if it's be worth factoring this into the derived compare composed with a function which strips source spans. It'd mean two traversals, but you'd get a function which would probably be useful elsewhere (for similar optimizations or other things). If the generic annotations work gets merged later, it'd literally become an fmap or something similar.

I'd hesitate for a couple reasons:

This is personal preference and I realize I am probably slower to do this than is typical for the average PureScript contributor, but I don't like generalizing until I have at least 2 or 3 applications of the generalization I can evaluate. Is there an example anywhere else in the test suite of compare being expensive? If there is, I'd be happy to poke at deduplicating. For the moment, it's extra work incorporating an approach I'm not terribly familiar with. I prefer not to add code that isn't yet needed to the compiler.

Looking at the exhaustiveness algorithm, we might be able to make it even more optimal if we use a Set instead of a list that we ordNub at every step of the computation.

@parsonsmatt that was the first approach I investigated and I think it was a good idea not to tackle that first. That's what I'm giving @MonoidMusician credit for helping me to avoid.

Here's the current state of the flamegraph:

See the skinny'ish tall tower on the left?

Zooming in, it's still ordNub eating us alive in the exhaustiveness check:

But it's still principally compare, this time on Name, that is killing us. At time of writing:

-- | A sum of the possible name types, useful for error and lint messages.
data Name
  = IdentName Ident
  | ValOpName (OpName 'ValueOpName)
  | TyName (ProperName 'TypeName)
  | TyOpName (OpName 'TypeOpName)
  | DctorName (ProperName 'ConstructorName)
  | TyClassName (ProperName 'ClassName)
  | ModName ModuleName
  | KiName (ProperName 'KindName)
  deriving (Eq, Ord, Show, Generic)

I think eliminating the nested lists of binders would be worth it iff it is the case that the number of times values are compared can be dramatically reduced by keeping them in a Set to begin with. It's possible a naive translation to Set [Binder] could suffice for that. I started a (messy) branch for this last night: https://github.com/bitemyapp/purescript/tree/callen/abortive-set-exhaustive-ordnub

I'm going to try to keep the change localized to the exhaustiveness check but it still seems like a bit of a briar patch. I'll poke more but I'd like to see this change get in independent of anything else as it seems like an easy and big win.

One suggestion I'd like to think about is if we could just make Eq and Ord on SourceSpans constantly return Eq. I doubt we ever want these spans to affect our equality checks.

If that's too radical, maybe we can use a newtype wrapper that skips comparisons for usage within our AST data types.

@kritzcreek I think that's a bad idea that doesn't have a GHC profile motivating it. If you don't actually want people to use an Eq or Ord instance, stop deriving them and fix all the code that implicitly depended upon them. Doing surprising things at runtime doesn't seem to me to be consistent with the design ethic I see in Haskell or PureScript.

I do not think exceeding the changes of this PR WRT SourceSpans is a good use of time unless someone can point out an example of the source location information causing a performance problem elsewhere.

Okay, yeah I think that's probably right. I was thinking about our Declaration type, but I'd first need a profile to actually show it causing problems. LGTM from me 👍

I'd value a comment about how the derived instance is different.

@puffnfresh Reasonable even if it's obvious what we're doing (dropping the source spans from the comparisons) -- rather leave a note explaining the purpose of the custom instance for posterity. I'll PR it later tonight.

@puffnfresh #3271