Lint for inefficient Quiver check statements

[alanrussian]

Describe the rule you'd like to see implemented

While profiling my team's code, I noticed we were spending a significant amount of time on constructing the message for check statements that we weren't even tripping. The reason was that we were using string interpolation and calling Type.toString() as part of these check statements. For example:

// Using quiver checks
checkState(someAlwaysTrueStatementInProd,
    message: '$SomeClass blah blah ${someExpensiveComputation()}');

This performance issue is easily fixable by making the message a lambda:

checkState(someAlwaysTrueStatementInProd,
    message: () => '$SomeClass blah blah ${someExpensiveComputation()}');

However, this is a common mistake developers make. We can prevent these performance regressions, and potentially speed up a bunch of existing code, by making a lint around this.

@davidmorgan suggested generalizing this lint by making a "must be const or function/lambda" lint. I like that idea and already know of other places we can reuse that in my team's codebase.

[srawlins]

I might be missing a lot of context here. What is the lint request? Is it just for this quiver function? Or is it generatlizable? Can we just change quiver's APIs?

[alanrussian]

We have several options:

1. Create an annotation for method arguments that must be a const string or function. Annotate quiver's message arguments with this annotation. Create lint that enforces the annotation.
2. Change the Quiver APIs to require that message arguments are always function.
3. Create a lint specific to Quiver that mandates inputs to Quiver's message arguments are const strings or functions.

IMO this is a generalizable problem that can be solved with (1) or (2) in all cases. I would prefer we don't implement this with (3).

(2) is a breaking change to Quiver, and I assume Quiver didn't implement things this way originally because it's a slightly inconvenient to always pass a lambda when it's frequently unneeded for performance?

Also, as I'm thinking about this some more, we probably need to be looser than const string or function, because a developer may want to pass an object to message.

[srawlins]

IMO this is a generalizable problem that can be solved with (1) or (2) in all cases. I would prefer we don't implement this with (3).

I agree; (1) or (2) sound good.

(2) is a breaking change to Quiver, and I assume Quiver didn't implement things this way originally because it's a slightly inconvenient to always pass a lambda when it's frequently unneeded for performance?

Yeah it would be interesting to see how much work it would be to make this change in internal google. I see 1600 calls. 🤷

We've previously made a lot of headway on an annotation like "@mustBeConst" but I don't think we completed anything. It was very difficult to track "constness" through a program. Especially w.r.t. String literals, const strings, interpolation, adjacent strings, helper functions.

[lrhn]

Even function literals are not free, you have to allocate a closure. I hope we are fairly efficient at that, so slightly faster than doing a string interpolation. But I wouldn't be certain.
(If we can determine that the condition is true at compile-time, the closure creation can be optimize away entirely. That only works if the check function is inlined, which it might be.)

I'd personally prefer just discouraging the check functions entirely.
Just write the if (!test) throw StateError("message"); out. It's cleaner! (And for arguments, you should always use the ArgumentError.value constructor, not just ArgumentError(message) like checkArgument does!)

[alanrussian]

Even function literals are not free, you have to allocate a closure. I hope we are fairly efficient at that, so slightly faster than doing a string interpolation.

IIRC the slowness is more from Type.toString() than the string interpolation.

(If we can determine that the condition is true at compile-time, the closure creation can be optimize away entirely. That only works if the check function is inlined, which it might be.)

It's not possible to determine this in all and maybe even most cases.

[srawlins]

Even function literals are not free, you have to allocate a closure. I hope we are fairly efficient at that, so slightly faster than doing a string interpolation.

IIRC the slowness is more from Type.toString() than the string interpolation.

If function literals are not that much cheaper than string interpolation, and they're less readable, maybe we should just focus on avoid_type_to_string. (Or a new lint rule, I honestly can't tell what that one does since the description references Type in passing, and the examples reference runtimeType. #58891)

[alanrussian]

I believe avoid_type_to_string would address '$SomeClass' but not '${someExpensiveComputation()}'. Both are a concern for performance.

…

On Tue, Jan 3, 2023 at 10:52 AM Sam Rawlins ***@***.***> wrote: Even function literals are not free, you have to allocate a closure. I hope we are fairly efficient at that, so slightly faster than doing a string interpolation. IIRC the slowness is more from Type.toString() than the string interpolation. If function literals are not that much cheaper than string interpolation, and they're less readable, maybe we should just focus on avoid_type_to_string <https://dart-lang.github.io/linter/lints/avoid_type_to_string.html>. (Or a new lint rule, I honestly can't tell what that one does since the description references Type in passing, and the examples reference runtimeType. dart-lang/sdk#58891 <#58891>) — Reply to this email directly, view it on GitHub <#58796>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ7IZ3UFWPHCLHPJQQ7EITWQRYOZANCNFSM54FNZWEQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>