Skip to content

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

Merged
sylvestre merged 1 commit into
uutils:masterfrom
nicoonoclaste:factor/rerecursify
Mar 20, 2021
Merged

Revert #1571 “perf/factor ~ deduplicate divisors”#1842
sylvestre merged 1 commit into
uutils:masterfrom
nicoonoclaste:factor/rerecursify

Conversation

@nicoonoclaste
Copy link
Copy Markdown
Contributor

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.
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

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)

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

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.

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

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

@sylvestre
Copy link
Copy Markdown
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)

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

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

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste 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.

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

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

@sylvestre sylvestre merged commit 8b9ac0c into uutils:master Mar 20, 2021
@nicoonoclaste nicoonoclaste 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