ISISNeutronMuon / ISISNeutronMuon/p4pillon

Rule efficiency improvements

Open
#62 0 comments 0 reactions 0 assignees View on GitHub
enhancement
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.