[ fix #2202 ] Use SnocList in parser state to avoid quadratic slowdown

[dagit]

No description provided.

[gallais]

Looks good. Can we please add the file that used to cause the slowdown as
a test in tests/idris2/perfXXX?

[dagit]

Looks good. Can we please add the file that used to cause the slowdown as a test in tests/idris2/perfXXX?

Sure. I added it as perf2202 (to watch the ticket). I wasn't sure if we have permission to use the original program, so I just made a quick tool to generate a file with definitions likes:

xNNN : Nat
xNNN = NNN

I generated 10k definitions, which takes a bit under 2 minutes before my changes and about 10 seconds after them. If it's too much we could trim it down, but under about 5k definitions and the slowdown is less obvious. At least on my test machine.

[mattpolzin]

which takes a bit under 2 minutes before my changes and about 10 seconds after them.

In my opinion, 2 seconds after your change and something notably slower before would be a sweet spot (10 seconds is a bit unfortunate when running the whole test suite locally to verify no regressions on a new branch).

[dagit]

If you know how many definitions you want in the file, I can shorten it and update the PR, but the power adapter for the computer where I was doing this caught on fire today. I won't be able to get a replacement until sometime next week. But if you know how big you want the file I can easily make that change right through the github UI.

Assuming the current algorithm is approximately linear, 2k definitions would be a good starting point for targeting 2seconds, but I don't know how obvious the regression will be at 2k.

[mattpolzin]

Let's take a stab at it and see where we land (your guess is as good as mine); we can see the resulting time in GitHub CI and use that as a gauge.

[dagit]

Right, but it won't show us the regression timing.

[dagit]

I cut it down to 2.5k but someone will need to reapprove the tests.

[mattpolzin]

I'll run the test on my machine against main and comment with what I see; as long as the difference is not negligible I am happy (but others can certainly disagree).

[mattpolzin]

On my machine, the results with/without the fix are now: 01.835s/03.500s.

In my opinion, you could step it up to 3k and see where we are at, but already that is a notable difference this is the sweet spot (see my next comment). Sure, 3.5s does not scream out "I'm exponential!" but if you compare it to 1.8s I'd say there's a regression in runtime that absolutely could be protected against (if, for example, we actually had guards in place in CI that prevented perf regressions).

[mattpolzin]

We are up to 02.594s in the GitHub CI so I bet the difference in time between with and without your fix would be even larger than that on my machine where I saw 01.835s runtime of the test with your fix applied.

[gallais]

I wasn't sure if we have permission to use the original program

@AndrasKovacs can we use your file in the CI process to guard against regressions?
Thanks!

the power adapter for the computer where I was doing this caught on fire today

😱

[AndrasKovacs]

@gallais Sure.

[mattpolzin]

@gallais are you requesting that the perf test get swapped out or just checking that we have permission to do so? I'm ok either way; having the test exhibit the perf fix and still not take too long with the newer fixed code is what is important to me.

The worst thing that can happen in my opinion is to let this PR languish instead of getting the fix in.

[gallais]

I'm in favour of adding both perf tests. I was planning to do it myself later this week
(I got up to 122 unread github threads over the holidays and am slowly going through them now I'm back).

[gallais]

I had to halve the file to keep the test time reasonable (~5s as opposed to ~15s whole)
so there's still an issue I guess.