fix Issue 19550 - [REG 2.078] Massive compiler backend slowdown

[WalterWaldron]

Fixes typo which caused a redundant O(N^2) slowdown.

I set the branch to master, I'll leave it to @MartinNowak to decide whether this should be in stable.

[dlang-bot]

Thanks for your pull request and interest in making D better, @WalterWaldron! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19550	regression	[REG 2.078] Massive compiler backend slowdown

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

1. Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

2. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#11085"

[thewilsonator]

cc @WalterBright

[MartinNowak]

2.078 is really old so this doesn't seem urgent. The current beta has also started and is a bit shorter, next release will follow suit on July 1st.
I don't understand this e = e.EV.E2 assignment, so wouldn't feel comfortable deciding this on my own.

[WalterWaldron]

I don't understand this e = e.EV.E2 assignment, so wouldn't feel comfortable deciding this on my own.

My understanding is as follows:
e.EV.E1 is the left hand side of a comma operator and e.EV.E2 is the right hand side.
Normally accumaecp recurses on both sides. I suspect this optimization is added to flatten the call graph in cases where DMD emits a long chain of comma operator.
For example, the code for bug #6401 no longer stack overflows with this patch (it is still extremely slow to compile due to a another O(N^2) issue.)

If adding a test is required, any help is appreciated since I don't know how to add performance tests to the test suite.

[WalterBright]

This is the way the code should have been written in the first place. @WalterWaldron caught my mistake. Thanks!

[ibuclaw]

If adding a test is required, any help is appreciated since I don't know how to add performance tests to the test suite.

@MoonlightSentinel - maybe we could add machinery to run expensive tests? We don't want to run tests that take >2 minutes to compile by default, it already takes long enough to run CI. Likewise, using timeout to enforce a limit on compile time may result in sporadic failures, unless we use a value high enough that it can never be a false positive (e.g 600 seconds)

[MoonlightSentinel]

We could use the same approach taken for runnable_cxx, i.e.reate a new test type (directory) and run them only on Github Actions/Azure.

Should this benchmark utility target compilable or all test types?

[ibuclaw]

We could use the same approach taken for runnable_cxx, i.e.reate a new test type (directory) and run them only on Github Actions/Azure.

Should this benchmark utility target compilable or all test types?

Well you have compile-time expensive, and run-time expensive. It makes no sense that a fail compilation test should ever take more than 5 seconds to error and bail out, let alone 2 minutes.

[MoonlightSentinel]

Agreed, fail_compilation tests shouldn't be affected. Having both compilable and runnable(_cxx) tests makes this more difficult but I'll think about it.

[ibuclaw]

Agreed, fail_compilation tests shouldn't be affected. Having both compilable and runnable(_cxx) tests makes this more difficult but I'll think about it.

First thing that comes to mind is DISABLED: expensive, then use an environment variable to override the disabling.

[MoonlightSentinel]

That works if we only want to disable expensive tests on certain test runners.

But given your previous point:

Likewise, using timeout to enforce a limit on compile time may result in sporadic failures, unless we use a value high enough that it can never be a false positive (e.g 600 seconds)

I would also consider a new test parameter which declares a timeout, e.g.

// Abort after 2 minutes
// TIMEOUT: 2min 

and maybe support a default value:

// Aborts aftfer 1 minute
// TIMEOUT:

Tests declaring this parameter could then be disabled as you suggested.

[schveiguy]

Please be cautious about adding timing based tests to CI. We do not want to see random PR failures because of environmental issues we have no control over on the various CIs.

I'd say a better metric is to measure the number of calls and ensure they don't go past a certain count (i.e. not quadratic).

[ibuclaw]

Please be cautious about adding timing based tests to CI. We do not want to see random PR failures because of environmental issues we have no control over on the various CIs.

This is why I said default disabled unless explicitly turned on in the environment.

I'd say a better metric is to measure the number of calls and ensure they don't go past a certain count (i.e. not quadratic).

This would require building the compiler with profiling turned on, and does not help with CI taking > 1 hour to run because of individual test files taking > 10 minutes to compile. It is already floating around the 35 minute mark on the old windows auto tester box.

[ibuclaw]

As the timing difference with/without this change is 1m5s -> 600ms, I don't think there can be any false positive if the compilation time regresses to being greater than 10-15 seconds again.

But again, testing this as part of the regular CI is not something I'd recommend, however much we care about compiler performance.

[schveiguy]

As the timing difference with/without this change is 1m5s -> 600ms

Ah, that's a bit different than what I was thinking. Yeah, saying something like 10 seconds is probably fine. But it will happen that some PRs fail if this is part of CI.

It is annoying these days we have to restart a lot of CI tests, many times just due to environmental failures.

This is why I said default disabled unless explicitly turned on in the environment.

then it becomes something that's not tested, i.e. useless.