letsencrypt
va: Avoid creating multiple slow remote timers in doRemoteOperation
The code that sets a tighter deadline for slow remote VAs was inside a loop, causing a new timer (and goroutine) to be created on every iteration after quorum was reached. While not incorrect (only the first timer's cancel matters), this was wasteful.
Add a slowTimerSet flag to ensure only one timer is created.
Benchmark results show the fix is ~6.7x faster with ~7x less memory:
BenchmarkSlowRemoteTimerMultiple 1907 ns/op 1464 B/op 17 allocs/op
BenchmarkSlowRemoteTimerSingle 284 ns/op 208 B/op 3 allocs/op
(I didn't add these benchmarks in the PR because I just wanted to validate it's a big enough improvement)
Also, having said that, I'm having second thoughts on whether this is worth it - since this only triggers after a quorum of successes (or failures) has been reached, the number of remaining executions of the loop will always be quite low (and even if not, the number of perspectives is generally going to be less than 10).
Did this come up during profiling, or is this something you just happened to notice as "off" when reading the code?
or is this something you just happened to notice as "off" when reading the code?
This is where I noticed it. Realized that it might just be some unnecessary allocations.
I don't have a profiling situation setup, but I can try to get one up to see what it looks in real traffic use.
I think it's not worth you setting up profiling; I'm pretty sure we know the answer (it'll be minimal); so we should make the decision based on whether we think it makes the code cleaner and clearer. @aarongable you already took a look; what do you think?
I went through this exact same thought process when reviewing the original code, and decided that the potential extra timers weren't worth worrying about. But it was a very weakly-held opinion, and so when this PR popped up I was happy for it to go this way as well.
I think that having this little bit of extra tracking doesn't make the code harder to read, and does obviate the need for the reader to reason through the "what if multiple timers get started?" case. I do agree that slowTimerStarted would be a better name than slowTimerSet. I don't think that replacing the bool check with a nil check would be better; although it saves a variable I think it makes it less obvious what's happening.
