assert with constants where possible#4153
Conversation
Lots of places were asserting with a supplier of a constant String; switch to just using the assert methods that take the constant String directly. While the lambdas were cached / inlined, we can avoid even the small overhead of the lambda existing and needing the JIT to inline.
480096d to
4957f16
Compare
|
Hi @kilink ... thanks for this PR, but I am not sure we will merge this. Reason being is, that we changed it actually to this version some time ago. We also did run benchmarks and there is no difference in supplier or no supplier version. @bbakerman might remember more. |
Sure, this PR isn't really about the performance implications. The minuscule difference wouldn't show up in a benchmark, since a benchmark is going to show performance once it's already JITed. The real argument I see here is that of readability / idiomatic code. It doesn't make much sense to me to have a lambda that yields a constant String, especially when there are variants of the assert methods that take constant Strings. |
|
Yeah we l looked at this a while back. When a Lambda has a constant value or more specifically when it encloses not outside variables then the Java compiler will inline the lambda code and will no "allocate" any object on each call. So the runtime memory costs of
Yeah this is where we get into the realm of what is sensible and what is code opinion. I agree that its probably more "idiomatic" for constant strings to be direct constant strings BUT we also non constant string assertions (albeit we tried very hard to avoid them for the reasons above on cost) So there is a consistency argument (code opinion) vs The top one is consistent in that all take suppliers while the bottom one is inconsistent in that if you are constant dont use a supplier but if you are non constant then do use one. Then you get into the idea of defaulting.... a supplier pattern is more likely to be efficient if you forget the rules and add a non constant alert message BUT all of this is countered by the fact that we aim for "constant" assert messages as much as possible because of efficiency. Code opinions are hard - lets go shopping! |
|
We have decide to accept this after discussing it more |
Lots of places were asserting with a supplier of a constant String; switch to just using the assert methods that take the constant String directly. While the lambdas were cached / inlined, we can avoid even the small overhead of the lambda existing and needing the JIT to inline.