uninstall: fix accidental cubic complexity #34005

haampie · 2022-11-18T19:16:48Z

Currently spack uninstall runs in worst case cubic time complexity
thanks to traversal during traversal during traversal while collecting
the specs to be uninstalled. This fix makes it linear time.

Also reduce the number of error messages to something that scales
linearly with the number of specs to be uninstalled, instead of
quadratically worst case.

Adapted from #34001 without the behavioral changes, so we can
backport as a bugfix.

Currently spack uninstall runs in worst case cubic time complexity thanks to traversal during traversal during traversal while collecting the specs to be uninstalled. Also brings down the number of error messages printed to something linear in the amount of matching specs instead of quadratic.

haampie · 2022-11-18T19:20:51Z

Note: I did not really check if behavior changes here w.r.t. missing specs listed in the database. We also have no tests covering that, maybe it's harmless to run uninstall on an already missing spec? I leave that for the reviewer.

haampie · 2022-11-18T19:51:41Z

lib/spack/spack/cmd/uninstall.py

@@ -158,19 +159,22 @@ def installed_dependents(specs, env):

    env_hashes = set(env.all_hashes()) if env else set()

-    all_specs_in_db = spack.store.db.query()
+    # Ensure we stop traversal at input specs.
+    visited = set(s.dag_hash() for s in specs)


@scheibelp it would be simpler to implement this with BFS, since then it's simply traverse_nodes(specs, root=False, order="breadth") instead of doing DFS on the children. The only issue is that with BFS we can't know the associated input spec in constant time. So, for backporting purposes this ugliness suffices, but to be honest, I seriously doubt anybody is really interested in this very detailed error message about how [matching spec i] needs [dependent spec j] to be uninstalled. So, in the other PR I opted for just collecting all dependents in a flat list, instead of keeping track of what spec they are a dependent of.

bernhardkaindl

@haampie: I got warnings when using this to uninstall m4 from two groups, where mention dependent hashes (keax6xa and i2oq5x4) not being present in the DB - that it uninstalled before:

bin/spack uninstall --all --dependents m4
==> The following packages will be uninstalled:

    -- linux-ubuntu20.04-neoverse_n1 / gcc@10.3.0 -------------------
    i2oq5x4 bison@3.8.2  vvc6jj4 krb5@1.19.3        n3466oq libtool@2.4.7  rucqs2u openssh@9.1p1
    j7qjw62 hwloc@2.8.0  txha6wo libpciaccess@0.16  hl3xujs m4@1.4.19      ifk6xhh pmix@4.1.2

    -- linux-ubuntu20.04-neoverse_n1 / gcc@11.1.0 -------------------
    keax6xa bison@3.8.2  ymswwq3 krb5@1.19.3        v3w5jkh libtool@2.4.7  vem4pb4 openssh@9.0p1
    ybtm33t hwloc@2.8.0  ddseank libpciaccess@0.16  w6ty3sh m4@1.4.19      mpg7nso pmix@4.1.2

==> Do you want to proceed? [y/N] <
==> Do you want to proceed? [y/N] y
==> Successfully uninstalled openssh@9.1p1%gcc@10.3.0+gssapi build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/rucqs2u
==> Successfully uninstalled openssh@9.0p1%gcc@11.1.0+gssapi build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/vem4pb4
==> Successfully uninstalled pmix@4.1.2%gcc@11.1.0~docs+pmi_backwards_compatibility~restful build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/mpg7nso
==> Successfully uninstalled pmix@4.1.2%gcc@10.3.0~docs+pmi_backwards_compatibility~restful build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/ifk6xhh
==> Successfully uninstalled bison@3.8.2%gcc@11.1.0 build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/keax6xa
==> Successfully uninstalled bison@3.8.2%gcc@10.3.0 build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/i2oq5x4
==> Successfully uninstalled libtool@2.4.7%gcc@11.1.0 build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/v3w5jkh
==> Successfully uninstalled libtool@2.4.7%gcc@10.3.0 build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/n3466oq
==> Successfully uninstalled krb5@1.19.3%gcc@11.1.0+shared build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/ymswwq3
==> Successfully uninstalled krb5@1.19.3%gcc@10.3.0+shared build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/vvc6jj4
==> Successfully uninstalled hwloc@2.8.0%gcc@10.3.0~cairo~cuda~gl~libudev+libxml2~netloc~nvml~oneapi-level-zero~opencl+pci~rocm build_system=autotools libs=shared,static arch=linux-ubuntu20.04-neoverse_n1/j7qjw62
==> Successfully uninstalled hwloc@2.8.0%gcc@11.1.0~cairo~cuda~gl~libudev+libxml2~netloc~nvml~oneapi-level-zero~opencl+pci~rocm build_system=autotools libs=shared,static arch=linux-ubuntu20.04-neoverse_n1/ybtm33t
==> Warning: Inconsistent state! Dependent keax6xajhfwexkrxllpy2t3b4pmou4tz of w6ty3shuozc7w2v6yybssnkaxecttlo7 not in DB
==> Successfully uninstalled m4@1.4.19%gcc@11.1.0+sigsegv build_system=autotools patches=9dc5fbd,bfdffa7 arch=linux-ubuntu20.04-neoverse_n1/w6ty3sh
==> Warning: Inconsistent state! Dependent i2oq5x4i6t6vnsbxzo3znb2xikodgtnb of hl3xujs5mdscl5f36vgzff4nt2nct3e6 not in DB
==> Successfully uninstalled m4@1.4.19%gcc@10.3.0+sigsegv build_system=autotools patches=9dc5fbd,bfdffa7 arch=linux-ubuntu20.04-neoverse_n1/hl3xujs
==> Successfully uninstalled libpciaccess@0.16%gcc@11.1.0 build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/ddseank
==> Successfully uninstalled libpciaccess@0.16%gcc@10.3.0 build_system=autotools arch=linux-ubuntu20.04-neoverse_n1/txha6wo

haampie · 2022-11-20T22:20:17Z

See #34001 point 3: currently it doesn't order the specs topologically, but that's fixed there. This PR is just about a performance issue.

It would be nice to backport the topo order change to 0.19, but it would add some new features too, so not sure if possible.

alalazo · 2022-11-21T13:39:28Z

lib/spack/spack/cmd/uninstall.py

+            if hash in env_hashes:
+                active_dpts.setdefault(spec, set()).add(dpt)
+            else:
+                outside_dpts.setdefault(spec, set()).add(dpt)


If I read the code correctly, these lists are now computed differently, and won't match the docstring description (if A is listed as a dependent of B it won't be listed anymore as a dependent of anything else). This though will only matter for error messages as they are now, not for actual uninstallations. Also, the error messages in this PR will just be less complete - not incorrect.

On the other hand we avoid doing the same traversals over and over, which is a win in terms of performance.

If my understanding is correct this LGTM.

Ah, I didn't check the docstring. So yes, listing specs at most once is intended behavior: it ensures linear runtime and smaller error messages, in particular when doing spack uninstall --all.

For differences in the error message using the same store, with develop:

with this PR:

Actually uninstalling everything in that store from outside environments, on develop:

$ time spack uninstall -ay [ ... ] real 0m6,314s user 0m5,957s sys 0m0,327s

and with this PR:

$ time spack uninstall -ay [ ... ] real 0m3,821s user 0m3,492s sys 0m0,308s

* uninstall: fix accidental cubic complexity Currently spack uninstall runs in worst case cubic time complexity thanks to traversal during traversal during traversal while collecting the specs to be uninstalled. Also brings down the number of error messages printed to something linear in the amount of matching specs instead of quadratic.

spackbot-app bot added commands core PR affects Spack core functionality labels Nov 18, 2022

haampie requested a review from scheibelp November 18, 2022 19:17

haampie added this to the v0.19.1 milestone Nov 18, 2022

haampie commented Nov 18, 2022

View reviewed changes

bernhardkaindl reviewed Nov 20, 2022

View reviewed changes

spackbot-app bot added the tests General test capability(ies) label Nov 21, 2022

haampie force-pushed the fix/cubic-complexity-in-spack-uninstall branch 2 times, most recently from 9dac70c to 395542c Compare November 21, 2022 10:45

deal with missing dependents

f92d95d

haampie force-pushed the fix/cubic-complexity-in-spack-uninstall branch from 395542c to f92d95d Compare November 21, 2022 10:45

alalazo reviewed Nov 21, 2022

View reviewed changes

alalazo previously approved these changes Nov 21, 2022

View reviewed changes

Update docstring

edcb6f1

haampie dismissed alalazo’s stale review via edcb6f1 November 21, 2022 14:13

alalazo approved these changes Nov 21, 2022

View reviewed changes

haampie merged commit 8ea366b into spack:develop Nov 21, 2022

haampie deleted the fix/cubic-complexity-in-spack-uninstall branch November 21, 2022 15:45

haampie mentioned this pull request Nov 21, 2022

Backports 0.19.1 #33966

Merged

20 tasks

greenc-FNAL mentioned this pull request Dec 1, 2022

Sync with upstream FNALssi/spack#72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uninstall: fix accidental cubic complexity #34005

uninstall: fix accidental cubic complexity #34005

haampie commented Nov 18, 2022 •

edited

Loading

haampie commented Nov 18, 2022

haampie Nov 18, 2022 •

edited

Loading

bernhardkaindl left a comment •

edited

Loading

haampie commented Nov 20, 2022 •

edited

Loading

alalazo Nov 21, 2022 •

edited

Loading

haampie Nov 21, 2022

alalazo Nov 21, 2022

alalazo Nov 21, 2022

uninstall: fix accidental cubic complexity #34005

uninstall: fix accidental cubic complexity #34005

Conversation

haampie commented Nov 18, 2022 • edited Loading

haampie commented Nov 18, 2022

haampie Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

bernhardkaindl left a comment • edited Loading

Choose a reason for hiding this comment

haampie commented Nov 20, 2022 • edited Loading

alalazo Nov 21, 2022 • edited Loading

Choose a reason for hiding this comment

haampie Nov 21, 2022

Choose a reason for hiding this comment

alalazo Nov 21, 2022

Choose a reason for hiding this comment

alalazo Nov 21, 2022

Choose a reason for hiding this comment

haampie commented Nov 18, 2022 •

edited

Loading

haampie Nov 18, 2022 •

edited

Loading

bernhardkaindl left a comment •

edited

Loading

haampie commented Nov 20, 2022 •

edited

Loading

alalazo Nov 21, 2022 •

edited

Loading