Skip to content

spec.py: add EMPTY_SPEC#51819

Merged
becker33 merged 2 commits intodevelopfrom
hs/fix/spec-when-allocs
Jan 12, 2026
Merged

spec.py: add EMPTY_SPEC#51819
becker33 merged 2 commits intodevelopfrom
hs/fix/spec-when-allocs

Conversation

@haampie
Copy link
Copy Markdown
Member

@haampie haampie commented Jan 7, 2026

Conditional dependencies are the exception, but all unconditional
dependencies allocate a when=Spec() object after the introduction
of toolchains. Trivial specs are also common in package metadata,
since almost all directives are now dictionaries keyed by when
condition, where the unconditional Spec() is popular as well.

tracemalloc analysis shows that during spack spec zlib, 61% of all
Spec objects that are garbage collected (by the cyclic garbage
collector, which are all specs ever) are trivial/empty.

This commit introduced a singleton spack.spec.EMPTY_SPEC = Spec(),
which is "immutable by convention", used in the hot spots.

Setup time reduces by 12.7%.

comparison

Some other nice results, using the E4S environment's lock file of 2.9MB, comparing against fe43834

Pickle size of list(e.roots())

  • before: 5.0MB
  • after: 3.0M (-40.0%)

Pickle and unpickle speed:

  • before: 0.1830s
  • after: 0.0703s (-61.6%)

Database and Environment loading speed:

  • before: 0.1853s
  • after: 0.1471 (-20.64%)

Spawned subprocess startup speed, passing the environment as arg

  • before: 0.5066s
  • after: 0.2227s (-56.0%)

To reproduce, see envbench.tar.gz


Before this commit:

Total Spec objects: 85803
Trivial (empty) Spec objects: 52878

Top 20 allocation sites for trivial (empty) Spec objects:
   COUNT | LOCATION
-------------------------------------------------------------------------------------
   26666 | $spack/lib/spack/spack/spec.py:1846
    8930 | $spack/lib/spack/spack/spec.py:1917
    7535 | $spack/lib/spack/spack/spec.py:761
    5210 | $spack/lib/spack/spack/spec.py:4440
    3904 | $spack/lib/spack/spack/directives.py:129
     532 | $spack/lib/spack/spack/solver/input_analysis.py:88
      77 | $spack/lib/spack/spack/spec.py:3771
      18 | $spack/lib/spack/spack/spec.py:4396
       1 | $spack/lib/spack/spack/solver/requirements.py:203
       1 | $spack/lib/spack/spack/solver/requirements.py:67
       1 | $spack/bin/spack:110

After this commit:

Total Spec objects: 33020
Trivial (empty) Spec objects: 99

Top 20 allocation sites for trivial (empty) Spec objects:
   COUNT | LOCATION
-------------------------------------------------------------------------------------
      77 | $spack/lib/spack/spack/spec.py:3771
      16 | $spack/lib/spack/spack/spec.py:4396
       1 | $spack/lib/spack/spack/solver/requirements.py:203
       1 | $spack/lib/spack/spack/solver/requirements.py:67
       1 | $spack/bin/spack:110

@github-actions github-actions bot added the solver label Jan 7, 2026
@haampie haampie changed the title spec.py: use EMPTY_SPEC for unconditional deps spec.py: add EMPTY_SPEC Jan 7, 2026
@haampie haampie force-pushed the hs/fix/spec-when-allocs branch from 077a96b to f20583d Compare January 7, 2026 13:40
@haampie

This comment was marked as duplicate.

Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only worrying part is the dance we do in with named_spec where we modify something that is logically const.

Can't we do:

diff --git a/lib/spack/spack/solver/asp.py b/lib/spack/spack/solver/asp.py
index aa489944ba..f26954ec16 100644
--- a/lib/spack/spack/solver/asp.py
+++ b/lib/spack/spack/solver/asp.py
@@ -133,6 +133,10 @@ def named_spec(
         yield spec
         return
 
+    # Clone empty spec when we need to assign a name
+    if spec is spack.spec.EMPTY_SPEC:
+        spec = spack.spec.Spec()
+
     old_name = spec.name
     spec.name = name
     try:
@@ -1842,7 +1846,9 @@ def _condition_clauses(
             if not imposed_name:
                 raise ValueError(f"Must provide a name for imposed constraint: '{imposed_spec}'")
 
-        with named_spec(required_spec, required_name), named_spec(imposed_spec, imposed_name):
+        with named_spec(required_spec, required_name) as required_spec_with_name, named_spec(
+            imposed_spec, imposed_name
+        ) as imposed_spec_with_name:
             # Check if we can emit the requirements before updating the condition ID counter.
             # In this way, if a condition can't be emitted but the exception is handled in the
             # caller, we won't emit partial facts.
@@ -1850,22 +1856,32 @@ def _condition_clauses(
             condition_id = next(self._id_counter)
             requirement_context = context.requirement_context()
             trigger_id = self._get_condition_id(
-                required_spec, cache=self._trigger_cache, body=True, context=requirement_context
+                required_spec_with_name,
+                cache=self._trigger_cache,
+                body=True,
+                context=requirement_context,
             )
-            clauses.append(fn.pkg_fact(required_spec.name, fn.condition(condition_id)))
+            clauses.append(fn.pkg_fact(required_spec_with_name.name, fn.condition(condition_id)))
             clauses.append(fn.condition_reason(condition_id, msg))
             clauses.append(
-                fn.pkg_fact(required_spec.name, fn.condition_trigger(condition_id, trigger_id))
+                fn.pkg_fact(
+                    required_spec_with_name.name, fn.condition_trigger(condition_id, trigger_id)
+                )
             )
-            if not imposed_spec:
+            if not imposed_spec_with_name:
                 return clauses, condition_id
 
             impose_context = context.impose_context()
             effect_id = self._get_condition_id(
-                imposed_spec, cache=self._effect_cache, body=False, context=impose_context
+                imposed_spec_with_name,
+                cache=self._effect_cache,
+                body=False,
+                context=impose_context,
             )
             clauses.append(
-                fn.pkg_fact(required_spec.name, fn.condition_effect(condition_id, effect_id))
+                fn.pkg_fact(
+                    required_spec_with_name.name, fn.condition_effect(condition_id, effect_id)
+                )
             )
 
             return clauses, condition_id
@@ -2028,14 +2044,14 @@ def package_splice_rules(self, pkg):
         for i, (cond, (spec_to_splice, match_variants)) in enumerate(
             sorted(pkg.splice_specs.items())
         ):
-            with named_spec(cond, pkg.name):
-                self.version_constraints.add((cond.name, cond.versions))
+            with named_spec(cond, pkg.name) as cond_with_name:
+                self.version_constraints.add((cond_with_name.name, cond_with_name.versions))
                 self.version_constraints.add((spec_to_splice.name, spec_to_splice.versions))
                 hash_var = AspVar("Hash")
-                splice_node = fn.node(AspVar("NID"), cond.name)
+                splice_node = fn.node(AspVar("NID"), cond_with_name.name)
                 when_spec_attrs = [
                     fn.attr(c.args[0], splice_node, *(c.args[2:]))
-                    for c in self.spec_clauses(cond, body=True, required_from=None)
+                    for c in self.spec_clauses(cond_with_name, body=True, required_from=None)
                     if c.args[0] != "node"
                 ]
                 splice_spec_hash_attrs = [
@@ -2052,17 +2068,23 @@ def package_splice_rules(self, pkg):
                             filt_match_variants.add(k)
                     filt_match_variants = sorted(filt_match_variants)
                     variant_constraints = self._gen_match_variant_splice_constraints(
-                        pkg, cond, spec_to_splice, hash_var, splice_node, filt_match_variants
+                        pkg,
+                        cond_with_name,
+                        spec_to_splice,
+                        hash_var,
+                        splice_node,
+                        filt_match_variants,
                     )
                 else:
                     if any(
-                        v in cond.variants or v in spec_to_splice.variants for v in match_variants
+                        v in cond_with_name.variants or v in spec_to_splice.variants
+                        for v in match_variants
                     ):
                         raise spack.error.PackageError(
                             "Overlap between match_variants and explicitly set variants"
                         )
                     variant_constraints = self._gen_match_variant_splice_constraints(
-                        pkg, cond, spec_to_splice, hash_var, splice_node, match_variants
+                        pkg, cond_with_name, spec_to_splice, hash_var, splice_node, match_variants
                     )
 
                 rule_head = fn.abi_splice_conditions_hold(

to clone the EMPTY_SPEC in the context manager?

@alalazo alalazo self-assigned this Jan 7, 2026
@haampie
Copy link
Copy Markdown
Member Author

haampie commented Jan 7, 2026

The only worrying part is the dance we do in with named_spec where we modify something that is logically const.

I agree. I'm reasonably sure that the suggested change un-does the gains of the PR :p unless we introduce YET_ANOTHER_EMPTY_SPEC_EXCEPT_FOR_THE_NAME_ATTRIBUTE = spack.spec.Spec() for that context manager. But that's not great either.

I could have a look how feasible it is to get rid of the temporary modification, and instead just pass the name along with the spec.

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Jan 7, 2026

I'm reasonably sure that the suggested change un-does the gains of the PR

Why? I think it should be local to the context manager (so not affecting pickling or subprocess startup etc.) 🤔 I can measure later

@haampie
Copy link
Copy Markdown
Member Author

haampie commented Jan 7, 2026

Edit: misread it.

You can do that, but I don't like it:

  1. spec vs spec_with_name variables inside the same function, easy to miss they are distinct only sometimes.
  2. they bring back another 1000+ redundant Spec()s which the PR tries to eliminate. Simpler would then be to not use EMPTY_SPEC in directives.py's _make_when_spec.

I think I'll try to eliminate the temporary spec.name = ... mutation.

@haampie haampie force-pushed the hs/fix/spec-when-allocs branch 3 times, most recently from 9d2d625 to 344f7a7 Compare January 9, 2026 16:19
@haampie
Copy link
Copy Markdown
Member Author

haampie commented Jan 11, 2026

Okay, I added _EmptySpec. Now #51821 is required for this PR to work because named_spec will raise.

Conditional dependencies are the exception, but all unconditional
dependencies allocate a `when=Spec()` object after the introduction of
toolchains. Trivial specs are also common in package metadata, since
almost all directives are now dictionaries keyed by `when` condition,
where the unconditional `Spec()` is popular as well.

tracemalloc analysis shows that during `spack spec zlib`, 61% of all
`Spec` objects that are *garbage collected* are trivial/empty. (I'm not
counting at the time of allocation, where some specs are temporarily
empty, but then mutated by the parser).

This commit introduced a singleton `spack.spec.EMPTY_SPEC = Spec()`,
which is "immutable by convention", used in the hot spots listed below.

Before this commit:

```
Total Spec objects: 85803
Trivial (empty) Spec objects: 52878

Top 20 allocation sites for trivial (empty) Spec objects:
   COUNT | LOCATION
-------------------------------------------------------------------------------------
   26666 | $spack/lib/spack/spack/spec.py:1846
    8930 | $spack/lib/spack/spack/spec.py:1917
    7535 | $spack/lib/spack/spack/spec.py:761
    5210 | $spack/lib/spack/spack/spec.py:4440
    3904 | $spack/lib/spack/spack/directives.py:129
     532 | $spack/lib/spack/spack/solver/input_analysis.py:88
      77 | $spack/lib/spack/spack/spec.py:3771
      18 | $spack/lib/spack/spack/spec.py:4396
       1 | $spack/lib/spack/spack/solver/requirements.py:203
       1 | $spack/lib/spack/spack/solver/requirements.py:67
       1 | $spack/bin/spack:110
```

After this commit:

```
Total Spec objects: 33020
Trivial (empty) Spec objects: 99

Top 20 allocation sites for trivial (empty) Spec objects:
   COUNT | LOCATION
-------------------------------------------------------------------------------------
      77 | /home/harmen/spack/lib/spack/spack/spec.py:3771
      16 | /home/harmen/spack/lib/spack/spack/spec.py:4396
       1 | /home/harmen/spack/lib/spack/spack/solver/requirements.py:203
       1 | /home/harmen/spack/lib/spack/spack/solver/requirements.py:67
       1 | /home/harmen/spack/bin/spack:110
```

Pickle size of `list(e.roots())`

* before: 5.0MB
* after: 3.0M (-40.0%)

Pickle and unpickle speed:

* before: 0.1830s
* after: 0.0703s (-61.6%)

Database and environment loading speed:

* before: 0.1853s
* after: 0.1471 (-20.64%)

Spawned subprocess startup speed, passing the environment as arg

* before: 0.5066s
* after: 0.2227s (-56.0%)

Signed-off-by: Harmen Stoppels <[email protected]>
Signed-off-by: Harmen Stoppels <[email protected]>
@haampie haampie force-pushed the hs/fix/spec-when-allocs branch from a38edd5 to ae1861e Compare January 12, 2026 08:31
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm the speed-up on setup:

Image

I'll let @becker33 give the final review and merge.

@haampie haampie requested a review from becker33 January 12, 2026 10:41
@becker33 becker33 merged commit 6284f0d into develop Jan 12, 2026
31 of 32 checks passed
@becker33 becker33 deleted the hs/fix/spec-when-allocs branch January 12, 2026 19:24
@haampie haampie mentioned this pull request Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants