bugfix: spec_dict_to_json caches bottom-up; disable caching spliced specs

[tgamblin]

spec_dict_to_json avoids making yet another node identifier for specs by using DAG hashes, but it neglected to cache them bottom-up. If there are many abstract specs in a DAG/env/etc., this means we get exponential recomputation of hashes. This could cause extremely long concretization times for large DAGs.

Also, spliced specs can potentially encounter a traversal bug where reused spliced specs can have dangling hashes. Avoid spliced specs for now and just disable caching for them since they are rare.

Effect on the E4S stack in CI:

See spack/spack-packages#5097.

spec_dict_to_json time for diamond DAGs before, with recomputation issues:

depth= 1 nodes=  4 paths=2  0.001s
depth=14 nodes= 43 paths=16384  9.629s
...
😬

After:

depth= 1 nodes=  4 paths=2  0.000s
depth=14 nodes= 43 paths=16384  0.001s
depth=39 nodes=118 paths=549755813888  0.004s

This is a minimal backport; a full refactor of traverse_* can go on develop, but this is intended to be low-risk for 1.2.2.

• Cache hashes bottom-up in spec_dict_to_json using traverse_nodes()
• spec_dict_to_json now raises if it encounters spliced specs, and the result is not cached.
• Add a test.

[haampie]

I'm not a fan of the iterative traverse_nodes approach, it makes it hard to reason about ordering.

I would either write a proper DFS implementation in this module that treats Spec -> Spec.build_spec as an edge and test it, or add build_spec support to traverse.py, and certainly eliminate the second and third call to traverse_nodes() in spec_dict_to_json.

Now it's a weird hybrid, and there shouldn't be comments about "bottom up" cause that's misleading.

[becker33]

1. _compute_specs_from_answer_set already has to figure out what the roots of the solve are. If we make the logic from there reusable (and still cache it on the Result object) we get the roots “for free”
2. Spec._to_dict and friends are already compute hashes bottom-up and include build specs. To whatever extent they aren't bottom-up that's a performance problem in the non-forcing case that we should fix, instead of working around it here duplicating work.

Is there a reason we can't rework this to reuse that existing infrastructure, maybe with some additional arguments to pass the force argument to dependencies in Spec hashing methods?

I would think any performance work we need to do on the hashing ordering there would be good to do anyway (e.g. if we need to do a post-order traversal instead of a pre-order traversal to avoid needing to pass the force argument and then reverse the nodes into the dict).

[haampie]

Another option is to disable the feature for spliced specs and backport that.

[tgamblin]

@haampie ok, caching is disabled for spliced specs, and the traversal logic is simpler. I think this is a decent backport. the diff looks big but some code just moved into a try/finally block.

@becker33:

Is there a reason we can't rework this to reuse that existing infrastructure, maybe with some additional arguments to pass the force argument to dependencies in Spec hashing methods?

I looked at this, and it seemed very invasive given the number of places that need to be updated to pass along force=True, but it might be the right way to go (in a follow-on, not here). force=True is only used in finalize_concretization and, now, in concretization caching, but it would be for a similar reason in both places.

I am not convinced that hashing is the right way to key abstract specs in serialization, so that's another reason I didn't do the necessary surgery when refactoring the concretization cache. It would be nice if we could just use id() or something instead of hashes, but the Spec JSON schema doesn't have a way to represent that. It expects a hash... well, a string, but I didn't want to test what assumptions we make about that string -- I suspect we expect it to be a hash in some places.

1. _compute_specs_from_answer_set already has to figure out what the roots of the solve are. If we make the logic from there reusable (and still cache it on the Result object) we get the roots “for free”

I'm not sure I see where this relates to concretization caching (which needs to pair up specs and node ids).

2. Spec._to_dict and friends are already compute hashes bottom-up and include build specs. To whatever extent they aren't bottom-up that's a performance problem in the non-forcing case that we should fix, instead of working around it here duplicating work.

I think the right near-term refactor here is to make traverse_*() have a build_specs=True option and have to_dict() use it, which would consolidate the logic and make it more broadly usable elsewhere. Can do that in a follow-on.

[haampie]

Bit odd that SpecSyntaxError is not a SpecError 🤔