I've looked a bit deeper, and it turns out that the "bug" isn't really a bug - essentially what bytestring is doing here is re-exporting findSubstring from Data.ByteString from Data.ByteString.Char8. More details in the ticket (Trac #15337).

Well, here's the relevant bit from the build log:

In D4915#135418, @bgamari wrote:

Yes, I agree that this is the right way forward. This also means that we can simply remove split-objs, since Windows is AFAIK the only platform that still used it.

Looks like the build fails because it can't find some .o files. Might want to look into that.

Maybe check out how this relates to D4929? There doesn't seem to be any overlap between the two patches, but D4929 puts foldl' in GHC.Prelude, which make the Data.List import redundant here.

Would it be possible to see the performance improvements reflected in the test suite somewhere? E.g., by adding a test that compiles a program that would benefit a lot from this in terms of compilation time?

• Please fix the parser error / build failure
• A more descriptive commit message would be great

Apart from the stylistic remarks; I wonder whether this change would pull its weight. The Mach- prefix doesn't seem entirely outrageous to me (after all, these are things that are supposed to be "close to the metal" or "in machine format"), and changing them has a bit of an impact wrt future patches, merging, etc. But I really can't judge which way the balance would tip.

What about those failing CI tests? I can't make perfect sense of it, but IIUC, one test now allocates 16% less (which is to be expected, I think?), while another test now fails.

I think this deserves a proper discussion in a Trac issue, with some background information and concrete motivating examples.

In D4820#133110, @simonpj wrote:

Thanks for doing this!

The lack of INLINE breaks stream fusion, leading to degeneration in performance for Int32, Int64 and similar types

Do you understand *why* the lack of INLINE breaks fusion. It's be really good to give a little example to illustrate the steps, and where they break. Otherwise it's too much like alchemy... "try it and see".

Also could you add this info in a Note? Otherwise someone in 5 years time might wonder what those INLINE pragmas are doing, and remove them. (Yes, if they are careful enough they could do a git-annotate, but it's not so clear.)

In D4827#133290, @Ninjatrappeur wrote:

I am a bit puzzled by the CI failure. I don't get why the runner cannot run ./validate.

Did I mess something up here?

Is there some ticket associated with this? Do we know the context in which it allocated too much? *Someone* has clearly run into this in the past, and before undoing the "fix", I'd like some assertion that the problem has actually gone away, rather than us just not running it in the circumstances that trigger it.

Fantastic, thanks a lot.

Abandoning in favor of D4830.

This solves the same problem as D4833, but seems like a more elegant solution, so I suggest we roll with this patch instead of mine.

In D4833#133260, @sighingnow wrote:

@tdammers We have already disabled -fdefer-type-errors and -fdefer-typed-holes in GHCi (by Phab:D1527), see https://github.com/ghc/ghc/blob/master/compiler/typecheck/TcRnDriver.hs#L2070. I opened a ticket Trac #15259 yesterday to disable -fdefer-out-of-scope-variables as well, and fixed it by Phab:D4830. After doing that, the program given in Trac #14963 prints the result correctly and doesn't panic.

In D4833#133276, @sighingnow wrote:

I think it's ok to allow -fdefer-type-errors in files to be load to ghci, which wouldn't cause a panic and is very helpful to develop and try partial finished programs. We only need to disable -fdefer-type-errors (as well as the other two -fdefer) to interactive inputs in ghci.

• Disable error deferring in interactive statements
• Documentation
• Test for Trac #14963 workaround
• Updated test output

• Override interactive dynflags, not session
• Updated test output

Your test is failing on OS X; seems to be a difference in how the wc implementations format their output - the Linux one left-aligns the 0, while the OS X one pads it with spaces. A not very elegant, but effective, solution would be to throw a quick sed job into the pipeling, something like | sed -e 's/^\s*//' maybe?

Might be confusing for an unsuspecting reader to read hints about swapping the branches of a conditional, and then seeing code that also reduces conditionals down to just the plain branches.

Updated to reflect changed output in tests, and adding -Werror to tests that should fail.

Marking as "request changes", because this is clearly WIP.

Do we know what causes these? Shouldn't we have a ticket about this?

ghc-pkg: properly recompute abi-depends for updated packages.

Ömer suggested that this build error may be related: https://gist.github.com/tdammers/259593ba48374fa1b3e0885ada646d08

Fix as per Trac #15019.

• Add SCCs to hunt down Trac #14683
• Add regression test (Grammar.hs from Trac #14683)
• Improve coercionKind(Role) perfomance
• Cache coercion roles in NthCo

• Added SCCs to hunt down Trac #14683
• Improve coercionKind(Role) perfomance
• Cache coercion roles in NthCo

• Bump perf test targets

Separate patch with "debug only" improvements in D4570.

• Fix bug in invocation of decomposeFunCo

In D4394#126353, @simonpj wrote:

I have fixed the bug and pushed to wip/tdammers/D4394

Rewrote commit message & implemented suggested change.

@simonpj, could you quickly review & comment on the updated summary (identical to commit message)?

Oh, and BTW, this is D4394 and has its own branch, wip/tdammers/D4394. The error can be reproduced by running validate on this branch.

I suppose I can check out wip/tdammers/D4395-new, build it, and try to fix it, but in what way does it not work?

Apparently, the way I tried to fix rebasing-induced errors wasn't correct, but unfortunately I don't understand the code well enough to see where I went wrong. Maybe @goldfire or @simonpj could take a peek?

Discard reflexive casts during Simplify

• Fix rebasing-induced failures

More specifically, I broke something, but e3dbb44f53b2f9403d20d84e27f187062755a089 also did - that commit (currently in master) removes the nextRole export from TcType.hs, which this patch relies on.

In D4395#125855, @goldfire wrote:

Wait a tick. Before your most recent commits, did addCoerce check isReflexiveCo twice? That's ridiculous (and my fault, I'm sure). That could be the reason for the slowdown.

Looks like I broke something when rebasing onto master.

• Fixed NthCo Note (discussion in D4385)
• Documented invariant on cached roles
• Tighten cached role in NthCo
• NthCo handling, assertions and documentation improvements
• Fixes to Coercion.hs
• Removed dead code
• Refactored setNominalRole_maybe to avoid dragging role through recursion
• Another fix as per D4394
• Fixed unused variables
• Refactor mkCoCast
• Fix test to reflect changed NthCo ctor
• Increased allocation limit for T5321 on OS X
• Fix rebasing mistake

• Discard reflexive casts during Simplify
• Fix huge performance regression

• Fix huge performance regression

On further thought, I think it'd be better to get this landed first, and then look into further improvements.

Are you saying that, *with the code I copy/pasted in the previous comment*, the perf is actually *a lot worse* than before the patch? That seems very implausible; it's such a benign change. (But NB you must use the code I copy/pasted. Your patch has isReflexive twice.)

In D4395#125736, @simonpj wrote:

So it does indeed look like simplCast is still the culprit. I can re-run with the SCC added back in if you need solid proof (though this is rather time consuming).

Are you saying that, *with the code I copy/pasted in the previous comment*, the perf is actually *a lot worse* than before the patch? That seems very implausible; it's such a benign change. (But NB you must use the code I copy/pasted. Your patch has isReflexive twice.)

But if it doesn't fix the problem, is simplCast still at the top of allocation list? And where is that time going?

Where is the data that says this patch degrades performance? The data in the previous comment shows a huge improvement.

Cache coercion roles in NthCo and refactor coercionKindsRole

Hold back the horses, I've done some additional profiling, "just to make sure", and it turns out that, at least for the Grammar.hs test case that tripped this whole thing off, performance is actually getting over an order of magnitude *worse*, compared to GHC HEAD. I don't think we should land this just yet.

• Fix rebasing mistake

This should apply cleanly onto GHC HEAD (as of today)

• Fixed NthCo Note (discussion in D4385)
• Documented invariant on cached roles
• Tighten cached role in NthCo
• NthCo handling, assertions and documentation improvements
• Fixes to Coercion.hs
• Removed dead code
• Refactored setNominalRole_maybe to avoid dragging role through recursion
• Another fix as per D4394
• Fixed unused variables
• Refactor mkCoCast
• Fix test to reflect changed NthCo ctor
• Increased allocation limit for T5321 on OS X

Squash & remove commits that don't belong here

In D4395#123656, @goldfire wrote:

The git history seems confused here, which is perhaps my fault. I contributed the code in CoreOpt.hs and Simplify.hs, which is relevant for this patch. The other changes are unrelated (and are around caching NthCo roles, which is a separate optimization).

• Increased allocation limit for T5321 on OS X

In D4394#124925, @simonpj wrote:

when I compile the relevant file with HEAD and the D4394 version, we see the massive improvement we expect

Are you saying that the 5.5% allocation lossage shows up when you run the testsuite, but NOT when you run this test individually? That really is mysterious. Or are you saying something bad only happens on OSX which you do not have access to?

In D4394#124922, @simonpj wrote:

Oh bother!

That does come as a surprise. I really thought everything would be faster.

Hmm, one test now fails on OS X. The relevant test case T5321 allocates 5.5% more now, which is 0.5% more than our set limit of 5%. The build also succeeds on the other platforms, only OS X seems to be having trouble.

Rebased onto current master.

• Fixed NthCo Note (discussion in D4385)
• Documented invariant on cached roles
• Tighten cached role in NthCo
• NthCo handling, assertions and documentation improvements
• Fixes to Coercion.hs
• Removed dead code
• Refactored setNominalRole_maybe to avoid dragging role through recursion
• Another fix as per D4394
• Fixed unused variables
• Refactor mkCoCast
• Fix test to reflect changed NthCo ctor

• Fix test to reflect changed NthCo ctor

• Refactor mkCoCast

• Fix unused variables

Address review remarks

In D4394#123710, @simonpj wrote:

do you want to land this diff as-is, or do you want to address the issues noted first?

I meant the latter, assuming you are referring to my comments immediately above. They are all easy to deal with.

@simonpj I'm a bit confused; do you want to land this diff as-is, or do you want to address the issues noted first?