Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert #1571 “perf/factor ~ deduplicate divisors” #1842

Merged
merged 1 commit into from
Mar 20, 2021
Merged

Revert #1571 “perf/factor ~ deduplicate divisors” #1842

merged 1 commit into from
Mar 20, 2021

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Mar 18, 2021

It was a draft PR, not ready for merging, and its premature inclusion caused repeated issues, see 368f473 & friends.

This reverts commits 3743a3e, ce218e0, and b7b0c76.

Close #1841.

It was a draft PR, not ready for merging, and its premature inclusion
caused repeated issues, see 368f473 & friends.

Close #1841.

This reverts commits 3743a3e,
                     ce218e0, and
                     b7b0c76.
@nbraud
Copy link
Contributor Author

nbraud commented Mar 18, 2021

I ran the tests in a loop for 45 minutes, and confirmed there's no remaining bugs exposed by the tests (or at least, the ones left are highly unlikely to be hit)

@nbraud
Copy link
Contributor Author

nbraud commented Mar 18, 2021

Performance wise, this seems to be a ~3% regression, so the complexity/speed trade-off seems to be definitely there.
We can revisit that if/when I find a non-cursed way to formalise and implement that GCD tree idea.

@nbraud
Copy link
Contributor Author

nbraud commented Mar 18, 2021

@sylvestre the CI failure seems unrelated to my changes, or factor at all

@sylvestre
Copy link
Sponsor Contributor

are you sure?
it seems clearly factor

PASS: tests/factor/t20.sh
gawk: cmd. line:1: (FILENAME=- FNR=168) fatal: node.c:415:make_str_node: r->stptr: can't allocate -2147483645 bytes of memory (Cannot allocate memory)

@nbraud
Copy link
Contributor Author

nbraud commented Mar 19, 2021

it seems clearly factor

PASS: tests/factor/t20.sh
gawk: cmd. line:1: (FILENAME=- FNR=168) fatal: node.c:415:make_str_node: r->stptr: can't allocate -2147483645 bytes of memory (Cannot allocate memory)

Thanks! I'm not sure how I missed that

@nbraud
Copy link
Contributor Author

nbraud commented Mar 19, 2021

@sylvestre It's also broken on master :

PASS: tests/factor/t20.sh
gawk: cmd. line:1: (FILENAME=- FNR=168) fatal: node.c:415:make_str_node: r->stptr: can't allocate -2147483645 bytes of memory (Cannot allocate memory)

Generally, there seems to be a lot of failures in that testsuite, so something is really wrong.

PS: I had a look, and the “Run GNU tests” task seems to just pull in whatever is in coreutils/coreutils, so pushes to the GNU repo can break it. Given that, and that it's broken on master anyhow, I would suggest getting rid of that task (at least until someone fixes it up) ; CI that we teach people to ignore, is worse than no CI.

@nbraud
Copy link
Contributor Author

nbraud commented Mar 19, 2021

Force pushed to remove the empty commit I used to rerun CI

@sylvestre sylvestre merged commit 8b9ac0c into uutils:master Mar 20, 2021
@nbraud nbraud deleted the factor/rerecursify branch March 20, 2021 14:50
@sylvestre sylvestre mentioned this pull request Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

factor test failures
2 participants