Optimization: Avoid quadratic behavior in IdMaker.make_unique_parameterset_ids

[NoxiousTab]

The Problem

IdMaker.make_unique_parameterset_ids (used to generate node ids for
@pytest.mark.parametrize) rebuilt set(resolved_ids) from scratch on
every candidate suffix it tried while de-duplicating colliding ids.
That set-rebuild is O(n), and it sat inside a loop over every parameter
set, making the whole de-duplication step O(n**2) in the number of
parameter sets whenever many of them produce the same id (e.g.
parametrizing with many values that stringify identically, or with
duplicate user-provided ids).

Fix

Build the set of in-use ids once, and update it incrementally as new
ids are assigned, instead of rebuilding it from the list on every
check. This brings the de-duplication step down to O(n).

Testing

• Verified with an isolated benchmark calling
  make_unique_parameterset_ids() directly with N colliding ids:
  runtime scaling went from ~4x per doubling of N (quadratic) to ~2x
  per doubling (linear), roughly 55x faster at N=8000.
• Existing testing/python/metafunc.py::TestMetafunc::test_idmaker_*
  tests pass unchanged.
• Full testing/python/ suite passes (629 passed, 24 skipped, 2 xfailed

[RonnyPfannschmidt]

Request changes

The cached-set idea is good (the set(resolved_ids) rebuild really is quadratic on heavy collisions), but the parallel set is only half-updated: new ids are added, old ids are never discarded. That changes node ids vs main, not just performance.

Required fix

When replacing resolved_ids[index], keep used_ids equivalent to the ids still present in the list (e.g. track a remaining Counter and discard the old id when its count hits 0). See the inline comment for a sketch.

Required test

Please add a regression test in testing/python/metafunc.py (near the other test_idmaker_* cases) that fails on this PR branch and passes on main / after the fix. Use the simplified counter-example from the inline comment, expressed via IdMaker + explicit ids:

def test_idmaker_unique_ids_frees_renamed_away_ids(self) -> None:
    """Suffix collision must see ids freed after all duplicates are renamed.

    Simplified resolved-id sequence: ["a10", "a10"] + ["a"] * 11
    After renaming both "a10" entries, "a10" must be available again for
    the 11th "a". A set that only .add()s new ids and never discards old
    ones incorrectly yields "a11" as the last id.
    """
    ids = ["a10", "a10"] + ["a"] * 11
    result = IdMaker(
        ("x",),
        [pytest.param(i) for i in range(len(ids))],
        None,
        ids,
        None,
        None,
    ).make_unique_parameterset_ids()
    assert result == [
        "a10_0",
        "a10_1",
        "a0",
        "a1",
        "a2",
        "a3",
        "a4",
        "a5",
        "a6",
        "a7",
        "a8",
        "a9",
        "a10",
    ]

Please do not land the optimization without that behavior lock-in.

[RonnyPfannschmidt]

This only half-updates the parallel set: resolved_ids[index] is replaced, but the old id is never removed from used_ids when it no longer occurs in the list.

That is not just an invariant smell — it changes assigned ids whenever a later candidate equals a fully-renamed-away earlier id.

Simplified counter-example (same algorithm as this loop):

# input
["a10", "a10"] + ["a"] * 11

# main today (rebuild set(resolved_ids) each probe)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a10"]

# this PR (stale "a10" left in used_ids)
["a10_0", "a10_1", "a0", "a1", "a2", "a3", "a4", "a5", "a6", "a7", "a8", "a9", "a11"]  # <-- last id wrong

After both "a10" entries are renamed, main frees "a10" for the 11th "a". Here "a10" stays reserved forever, so the last id becomes "a11".

Please keep used_ids in sync, e.g. with a remaining-count:

used_ids = set(resolved_ids)
remaining = Counter(resolved_ids)
...
remaining[id] -= 1
if remaining[id] == 0:
    used_ids.discard(id)
resolved_ids[index] = new_id
used_ids.add(new_id)

[NoxiousTab]

Done, using a remaining_id_counts copy of id_counts decremented per rename, discarding once it hits zero. kinda the same idea as your sketch. Traced through the counter example of ["a10","a10"]+["a"]*11 and confirmed in the code that it correctly produces "a10" as the last id.

[RonnyPfannschmidt]

used_ids = all_ids is just an alias of the same set object, so mutations below also mutate all_ids. Prefer a single name (used_ids = set(resolved_ids)), and once you add the remaining-count discard logic this alias becomes even more misleading.

[NoxiousTab]

Fixed, used set(all_ids) rather than set(resolved_ids). Since all_ids is already set(resolved_ids) computed a few lines up (used for the duplicate-count check), and resolved_ids has not been mutated yet at this point, set(all_ids) gives the identical set while avoiding rebuilding it from the last a second time.

Happy to switch it to set(resolved_ids) literally if its preferred to not depend on all_ids being untouched at this point in the function.