Fix hang in nmod_mpoly_gcd#2775
Merged
Merged
Conversation
d72eb6d to
55b15d7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I asked Fable 5 to fix #2581.
Its report is below. I did not ask it to follow up on its suggestion
since we currently have no other hanging examples, but someone else may want to investigate that.
Investigation of #2581 —
nmod_mpoly_gcdhangsReproduced on current master (
474f55e), p = 2^31 − 1, 12 variables. Baseline behavior:nmod_mpoly_gcdnmod_mpoly_gcd_zippelnmod_mpoly_gcd_zippel2nmod_mpoly_gcd_henselnmod_mpoly_gcd_brownWhere the hang is
All of the hanging entry points funnel into the same loop. Stack sample of
nmod_mpoly_gcd:_try_zippel2(and_try_hensel) begin by removing content in the two mainvariables; that content computation is a recursive
nmod_mpoly_gcd, whichselects classic Zippel — so
gcd,gcd_zippel2andgcd_henselall hang inthe same place. The "deflated polynomials" in the earlier comment are exactly
that inner content subproblem.
What the loop is doing
Instrumenting
nmod_mpolyu_gcds_zippel(the LINZIP sparse-interpolation step)shows that at recursion level
var = 3it returnsnmod_gcds_scales_not_foundon every single call:The caller
nmod_mpolyu_gcdp_zippeltreatsscales_not_foundas an unluckyevaluation point and retries with a new α (
goto inner_continue). Its looponly terminates when α wraps all the way around the field — with p ≈ 2^31 and
an rref per iteration, that is an effective hang (months of CPU).
Root cause:
l(number of images) is too small; the failure is structural, not bad luckDumping the failing subproblem (4 remaining variables; main variable X of
degree 2; coefficient supports of sizes 4, 8, 12) shows:
fmatches the gcd's support exactly,So neither the form nor content is the problem. Re-implementing the LINZIP
multiple-scaling linear system independently (same Kronecker-power evaluation
scheme, same per-block rref + stacked constraint matrix) shows that for this
support structure the system is generically rank-deficient at the image
count FLINT chooses:
FLINT computes
l = (Σ len_j + flen − 3)/(flen − 1), max'd with the largestblock, plus one test image — a naive equation count that assumes the stacked
constraint rows are generically independent. For this input they are not: two
images' worth of equations are dependent, so the system stays underdetermined
no matter which evaluation point is chosen. Retrying with new points can
therefore never succeed, but nothing in the code path ever increases
lor gives up:
nmod_mpolyu_gcds_zippelretries up to 10 fresh points internally, thenreturns
scales_not_found;nmod_mpolyu_gcdp_zippelresponds by picking yet another α and callinggcdsagain with the same form → the samel→ the same nullity — for upto p − 2 iterations.
Two independent defects
No escalation on underdetermination. LINZIP's prescription for an
underdetermined scale system is to add more images/equations. FLINT
restarts with a fresh point but an identical system size.
Unbounded retry of a non-transient failure. The inner loop in
gcdp_zippelbounds nothing except α wrapping the whole field. Capping theretries alone is not sufficient here: with only a retry cap, the failure
propagates to
nmod_mpolyu_gcdm_zippel's extension-field fallback, and theidentical LINZIP code in
fq_nmod_mpolyu_gcds_zippelhangs the same way(verified by stack sampling). The same duplicated logic lives in
src/nmod_mpoly/mpolyu_gcdp_zippel.candsrc/fq_nmod_mpoly/mpolyu_gcdp_zippel.c.Proof-of-concept fix
flint-issue2581-poc-fix.patchmodifies both copies ofgcds_zippel: whenthe system is still underdetermined after the existing two-point retry, free
the l-sized objects, grow
lby ~50% (up to 4 times), and re-run — instead ofreturning
scales_not_found.With the patch (nothing else changed), every previously hanging case
completes, all with the correct answer
l1*a2 − l1*a3 − l2*a1 + l2*a3 + l3*a1 − l3*a2:nmod_mpoly_gcdgcd_zippelgcd_zippel2gcd_henselRegression tests all pass:
nmod_mpoly_gcd,gcd_zippel,gcd_zippel2,gcd_hensel,gcd_cofactors,gcd_brown, andfq_nmod_mpoly_gcd,fq_nmod_mpoly_gcd_zippel,fq_nmod_mpoly_gcd_cofactors.A production fix should probably also bound the
scales_not_found/eval_point_not_found/eval_gcd_deg_too_highretries in bothgcdp_zippelinner loops (defense in depth — any other systematic failuremode currently loops ~2^31 times for a 31-bit modulus), and could grow
lmore carefully (e.g.
+flenper round). The fmpz_mpoly Zippel path reduces tothese nmod/fq_nmod kernels, so both benefit.
Regression tests
flint-issue2581-fix-and-tests.patchsupersedes the earlier PoC patch: itcontains the fix for both LINZIP copies plus regression tests in all six
affected test files. The test inputs are built compactly from the factored
forms (the deflated pair's 29-term common factor turns out to be exactly the
large cofactor of
h, up to variable renaming):nmod_mpoly/test/t-gcd.cnmod_mpoly/test/t-gcd_cofactors.cnmod_mpoly/test/t-gcd_zippel2.cnmod_mpoly/test/t-gcd_hensel.cnmod_mpoly/test/t-gcd_zippel.cnmod_mpolyu_gcds_zippeldirectlyfq_nmod_mpoly/test/t-gcd_zippel.cfq_nmod_mpolyu_gcds_zippeldirectlyValidation performed both ways:
reliably catch the bug, including the fq_nmod branch that a fix to only the
nmod copy would miss.
function), along with
nmod_mpoly_gcd_brownandfq_nmod_mpoly_gcdasbroader regression checks.