Rule timeouts
jedrichards opened this issue · 3 comments
- Feature should be configurable/optional, i.e. no limits in Node, limited in Sketch
- Assistant result should indicate whether its complete, or has been artificially limited. If it's been limited we won't be able to tell decisively whether the Assistant has "passed" or not, which may have UI implications - e.g. show when a result is indeterminate, explain why, and what to do about it.
Timeout
Introducing a timeout could lead results to be somewhat non deterministic, e.g. number of violations returned could vary depending on CPU load and how fast the rule executed. However it would be a good way to ensure that Assistant run time doesn't get totally out of control.
Considering that most rules should be capable of doing their thing quite efficiently, perhaps a 100ms timeout per rule could be considered? That would still be long enough for a rule to return 100s or even 1000s of violations depending on the document.
The downside would be rules that need to take a long time to run - could be annoying if they just timeout immediately before returning any results. But we could fine-tune this, or even eventually make it configurable in preferences.
Question - is the timeout per rule, per Assistant or per run? If each rule gets 100ms, but there are a ton of Assistants added to the document, with lots of rules each, then runtime could still start to get a bit long. Alternatively we could assign a time budget for the entire run, like max 4000ms, with each rule getting an equal slice of that.
Violation budget
The downside of a violation budget is it doesn't help with the case of rules that take an overly long time to run. If there's a violation budget of 200 per rule, but a rule is inefficient, then the Assistant runtime could still be way too long.
Also with efficiency improvements in the Sketch UI, then rendering large sets of violations doesn't seem to be an issue any more. So an efficient rule generating a good amount of violations before it timed-out doesn't seem like a problem any longer.
Thoughts @christianklotz @craig006
I think the timeout option covers more cases - Both assistants with many violations, and assistants that run slowly will be curbed. To me it makes sense that each assistant gets a set amount of time to use as they please. We could have an overall of 4000ms, like you said, which is nice to limit the overall runtime.
The violation budget I think is unfair towards assistants that are super efficient. :)
Filed #31067 as the timeouts don't appear to working for me
Moving this fella to Done and we will track the integration/problems in #31067.