Conversation
077a96b to
f20583d
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
alalazo
left a comment
There was a problem hiding this comment.
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?
I agree. I'm reasonably sure that the suggested change un-does the gains of the PR :p unless we introduce 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. |
Why? I think it should be local to the context manager (so not affecting pickling or subprocess startup etc.) 🤔 I can measure later |
|
Edit: misread it. You can do that, but I don't like it:
I think I'll try to eliminate the temporary |
9d2d625 to
344f7a7
Compare
|
Okay, I added |
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]>
a38edd5 to
ae1861e
Compare
alalazo
left a comment
There was a problem hiding this comment.
Can confirm the speed-up on setup:
- Spack: 1.2.0.dev0 (f861e2d)
- Builtin repo: spack/spack-packages@d4e3ea0
- Python: 3.12.3
- Platform: linux-ubuntu24.04-broadwell
I'll let @becker33 give the final review and merge.
Conditional dependencies are the exception, but all unconditional
dependencies allocate a
when=Spec()object after the introductionof toolchains. Trivial specs are also common in package metadata,
since almost all directives are now dictionaries keyed by
whencondition, where the unconditional
Spec()is popular as well.tracemalloc analysis shows that during
spack spec zlib, 61% of allSpecobjects that are garbage collected (by the cyclic garbagecollector, 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%.
Some other nice results, using the E4S environment's lock file of 2.9MB, comparing against fe43834
Pickle size of
list(e.roots())Pickle and unpickle speed:
Database and Environment loading speed:
Spawned subprocess startup speed, passing the environment as arg
To reproduce, see envbench.tar.gz
Before this commit:
After this commit: