va: Avoid creating multiple slow remote timers in doRemoteOperation

[aaomidi]

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)

[jsha]

This looks good. One nit: it's a little funny that we create and initialize a variable inside the bottom-most block, and set a variable tracking whether something (the variable?) has been "set", even though we immediately throw away the variable.

My inclination here would be one of:

• Call the bool slowTimerBegun.
• Replace the bool with var slowTimer *time.Timer, assign to that when we create the time, and use a nil check in place of the current bool check.

[jsha]

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?

[aaomidi]

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.

[jsha]

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?

[aarongable]

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.

[jsha]

Works for me 👍🏻