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.

[RonnyPfannschmidt]

one open question i have is - where did this show up - is this someting discovered in a performance bug or a llm scan of the project

[NoxiousTab]

one open question i have is - where did this show up - is this someting discovered in a performance bug or a llm scan of the project

neither actually, i was going through the heart of the project specifically looking for these kind of inefficiencies such as unnecessary nested loops, non-optimal choice of data structures, or redundant rebuilds like this, because these are easy to miss when the primary goal is correctness over nitty-gritty optimal details (which can still be costly), but later used llm to test the quadratic time hypothesis

[RonnyPfannschmidt]

so one question for this is - whats the actual speedup difference for common sizes of testsuites - an N of 8000 may be small in terms of algorithm complexity - but its also about 2-3 orders of decimal magnitude larger than what a good number of testsuites ever see per parameter

[NoxiousTab]

so one question for this is - whats the actual speedup difference for common sizes of testsuites - an N of 8000 may be small in terms of algorithm complexity - but its also about 2-3 orders of decimal magnitude larger than what a good number of testsuites ever see per parameter

thats a fair question, i ran a benchmark on main and then my branch for n = [10, 25, 50, 100, 250, 500, 1000] and got the following results

on main:
n= 10 time= 0.056ms
n= 25 time= 0.054ms
n= 50 time= 0.209ms
n= 100 time= 0.248ms
n= 250 time= 0.960ms
n= 500 time= 3.120ms
n= 1000 time= 10.424ms

my branch:
n= 10 time= 0.080ms
n= 25 time= 0.052ms
n= 50 time= 0.082ms
n= 100 time= 0.172ms
n= 250 time= 0.374ms
n= 500 time= 0.790ms
n= 1000 time= 1.535ms

At typical sizes (tens of parameters) the difference is in the noise, sub-millisecond either way, so this doesn't matter for most test suites. It only becomes measurable once a single parametrize call has a few hundred+ colliding ids, eg at n=1000 duplicate ids, ~10.4ms on main vs ~1.5ms on this branch, and the gap keeps widening since main is quadratic and this branch is linear.

It's real in theory, but admittedly narrower case rather than something every project will notice.
I'd understand if you'd rather not take this because its a fairly niche win. Happy with your call.

[RonnyPfannschmidt]

One practical details that puts this one to the grave for me is the fact that it's a net slowdown fir the actual common case

Practically even a single duplicate id is often a user error

Having more than a hundred is something more like a error not a algorithm problem to speed up

[RonnyPfannschmidt]

thank you for putting this detail to the test

the algorithm enhancement is real, and at the same time given the expected common case which measures slightly slower puts this one in the area where the theoretically better algorithm gives worse performance in the practical seize of N - n is best below 10 for pytest practical purposes - and its only really starting to be a issue for N>4000 as far as i can tell

i think on the algorithm front we might want to chase a different layer where we pre-cache parameter ids for specific parameterize markers/calls as some of those repeat per test item