ISISNeutronMuon / ISISNeutronMuon/p4pillon
Rule efficiency improvements
- Dominant language
- Python
- Stars
- 2
- Forks
- 0
- PR merge metrics
- No merged PRs in 30d
Description
# Intro
These linked issues were raised by Claude and text below is largely its.
## `is_applicable` is evaluated twice per array element, and `valueAlarm` limits are re-read per element
Profiling `ValueAlarmRule` on an NTScalarArray post turned up two sources of repeated work in the shared rules machinery. Both are real, both are outside any single rule, and both are fixed in `ScalarToArrayWrapperRule` / the `check_applicable_*` decorators rather than in the rule that exposed them.
### 1. Double applicability check — ~46% of a post
`ScalarToArrayWrapperRule._apply_elementwise` (`p4pillon/rules/rules.py:481-501`) calls the wrapped rule once per element:
```python
rule_flow = self._wrapped.post_rule(current_state, scalared_new_state)
```
`BaseRule.post_rule` is wrapped in `@check_applicable_post`, which calls `self.is_applicable(newpvstate)` (`rules.py:91`). The default `post_rule` body then delegates to `init_rule` (`rules.py:282`), which is wrapped in `@check_applicable_init` and calls `is_applicable` again (`rules.py:74`). So every element pays for two full applicability checks.
`is_applicable` (`rules.py:242-259`) is not cheap: a `set(...).issubset(newpvstate.keys())` plus a `Value.changed()` call per declared field, all crossing into p4p.
Measured: stubbing the second check took a representative post from **17.05 ms to 9.22 ms** — the single largest win available.
Worth noting that the answer cannot change between the two calls. The wrapper hoists a single `scalared_new_state` out of the loop and only reassigns `["value"]`, so the changed-set is fixed for the whole traversal. The check is not just duplicated per element, it is duplicated per element for a value that is constant across the entire array.
### 2. `valueAlarm` limits re-read per element — ~24%
`ValueAlarmRule.__alarm_from_limits` (`p4pillon/rules/value_alarm_rule.py:135-138`) reads eight `valueAlarm.*` fields out of the `Value` on every iteration:
```python
for alarm_type, op in cls.LIMIT_CHECKS:
severity = pvstate[f"valueAlarm.{alarm_type}Severity"]
if severity and op(value, pvstate[f"valueAlarm.{alarm_type}Limit"]):
```
These are per-*record* constants — the same eight values for all N elements — but they are fetched from p4p (and the f-strings rebuilt) N times.
Measured: hoisting them out of the loop took the same post from **16.97 ms to 12.82 ms**.
This is not fixable inside `ValueAlarmRule` alone without caching state on the rule instance, which rules are otherwise careful not to do. It needs a way for a gatherable rule to receive the array-constant portion of its inputs once per traversal rather than once per element.
### Acceptance
- [ ] Applicability is evaluated at most once per element per operation
- [ ] Array-constant rule inputs are read once per traversal, not once per element
- [ ] No change to any rule's observable behaviour; existing unit and integration suites pass
- [ ] The `# NOTE: Performance will be terrible!` comment at `rules.py:453` is updated or removed
Contributor guide
No contributing guide indexed for this repository
Research direction
Start by tracing ScalarToArrayWrapperRule._apply_elementwise and the check_applicable_* decorators in p4pillon/rules/rules.py, then inspect ValueAlarmRule.__alarm_from_limits in p4pillon/rules/value_alarm_rule.py. Run the existing unit and integration suites while checking the performance comment near rules.py:453. Done means each applicability check and array-constant input read occurs only as required, with observable behavior unchanged.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, performance
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100