Forder segfault

[ben-schwen]

Closes #4300

Changes recursive radix helper to an iterative version with a priority queue

• fix
• tests
• news
• timings

[MichaelChirico]

Hey @ben-schwen what's the status here? If you think the fix is good and the PR just needs to be wrapped up with tests/docs, we could ask for extra help there. It would be nice to include with 1.17.0 if you think it's doable.

[ben-schwen]

Hey @ben-schwen what's the status here? If you think the fix is good and the PR just needs to be wrapped up with tests/docs, we could ask for extra help there. It would be nice to include with 1.17.0 if you think it's doable.

While it fixes the linked issues, the patch in this branch runs into memory issues on other test cases, so not a real fix yet. The mentioned workaround with ulimit works on Linux until the stack size becomes too big for the memory.

@MichaelChirico it seems that #6274 has fixed the appearing segfaults when rewriting from a recursive to an iterative version by explicitly using more variable length arrays. So this can be patched up for 1.17.0. We should still watch out not to run into a performance regression

[github-actions]

Generated via commit f566f5d

Download link for the artifact containing the test results: ↓ atime-results.zip

Task	Duration
R setup and installing dependencies	4 minutes and 44 seconds
Installing different package versions	8 minutes and 6 seconds
Running and plotting the test cases	2 minutes and 27 seconds

[MichaelChirico]

Hey @ben-schwen what's the status here? If you think the fix is good and the PR just needs to be wrapped up with tests/docs, we could ask for extra help there. It would be nice to include with 1.17.0 if you think it's doable.

While it fixes the linked issues, the patch in this branch runs into memory issues on other test cases, so not a real fix yet. The mentioned workaround with ulimit works on Linux until the stack size becomes too big for the memory.

@MichaelChirico it seems that #6274 has fixed the appearing segfaults when rewriting from a recursive to an iterative version by explicitly using more variable length arrays. So this can be patched up for 1.17.0. We should still watch out not to run into a performance regression

that means this PR is still needed, but earlier #6274 cleared the way for this not to affect the other tests you mentioned? Or this bug is already fixed, and we only need this PR for regression testing?

[ben-schwen]

that means this PR is still needed, but earlier #6274 cleared the way for this not to affect the other tests you mentioned? Or this bug is already fixed, and we only need this PR for regression testing?

Yes and no. With #6274 we do not run into segfaults anymore. However, as shown in our GHA checks, forder can now fail because not enough memory can be allocated. This happens after multiple subsequent bmerge calls which could indicate that we have some memory leakage in forder