Specs are mutable, this breaks the invariant x == y => hash(x) == hash(y). A better approach is to make the hash function cheaper (at the possible cost of more collisions in dicts and sets)

Specs are mutable, this breaks the invariant x == y => hash(x) == hash(y). A better approach is to make the hash function cheaper (at the possible cost of more collisions in dicts and sets)

Tangent to this PR, but I started looking at the concretize setup time for my development environment as well and it spends a large fraction of time formatting Spec's to strings repeatedly for the same Spec's. I hacked in a similar change to the initial pass of caching the hash from this PR, but now caching the result of __str__. It would have some similar issues with Spec mutability I think, but it reduced concretize setup time from ~30s to ~20s.

Coming back to this (sorry for the delay).

Related to your last comment, I'm wondering whether we could define

class ImmutableSpec(Spec):
  def __hash__(self):
    try:
      return self._immutable_hash
    except AttributeError:
      h = super().__hash()
      self._immutable_hash = h
      return h
  # and the same for __str__

and then construct objects like these in directives.py.

Another possible advantage is that you can define the empty spec as a constant:

EmptySpec = ImmutableSpec()

and use that in directives that don't have a when condition, instead of constructing a new empty spec. That is advantageous because Python has a fast path in dicts and sets when x is y.

And regarding __str__, before caching it, it might be good to know what the call sites are. For all we know it's unused, for example in tty.debug(f"... {spec}") with debug messages disabled, but the string is constructed.

I've tried this PR, and my suggestion, but neither are significant improvements.

Also, just to confirm my original concern (no longer relevant after the change in this PR): we do mutate specs in various places, so freezing certain properties can be dangerous. I noticed the following examples:

• directives.py: when_spec.name = pkg.name
• asp.py: def named_spec -- here, after mutating name, __str__ is called

@vbrunini there have been many PRs recently aimed to speed up the Python bits of Spack, from #51819 to micro optimizations like #51841. Setup time has reduced by >25% over the last week.

We also socialized the idea that, by convention, Spec objects should be immutable, whether abstract or concrete. At this point:

• named_spec was removed, as well as another instance, so the solver is no longer mutating specs.
• directives.py mutation remains but is not particularly bad as it's part of initialization of a new Spec object.
• ClosedOpenRange was already immutable and does cache its str and hash value: version_types.py: cache ClosedOpenRange __str__ and __hash__ #51832

I'm still not sure whether caching __hash__ and __str__ is currently best, maybe it's too early. Also, it would be good to check with a histogram how often __str__ and __hash__ are actually called per instance.

Rebasing your PR I see about a 4.38% reduction in package load time (best of 5):

for i in $(seq 5); do time python3 -c 'import spack.repo; x = list(spack.repo.PATH.all_package_classes())'; done

For concretization I see a 6.3% reduction in setup time.

I would consider that significant.

Do you wanna revisit this PR?

I think something like this is sufficient?

        if not self._dependencies:  # fast path for trivial abstract specs (followed 94% in my tests)
            return hash((
                self.name,  # cheap
                self.namespace,  # cheap
                self.versions,
                self.variants if self.variants.dict else None,  # avoids expensive function call
                self.architecture,
                self.abstract_hash,  # cheap
            ))