Fix externally controlled executor not pausing #1479
Conversation
mstoykov
force-pushed
the
fixExternalExecutorNotPausing
branch
from
96ebd45
to
894ec1b
Compare
mstoykov
force-pushed
the
fixExternalExecutorNotPausing
branch
from
894ec1b
to
23955c5
Compare
Codecov Report
@@ Coverage Diff @@
## new-schedulers #1479 +/- ##
==================================================
+ Coverage 76.76% 76.80% +0.03%
==================================================
Files 163 163
Lines 13008 13061 +53
==================================================
+ Hits 9986 10031 +45
- Misses 2511 2517 +6
- Partials 511 513 +2
Continue to review full report at Codecov.
|
mstoykov
force-pushed
the
fixExternalExecutorNotPausing
branch
from
0125210
to
7fabed9
Compare
The problem was that pausing used gracefulStop which in turns wait for the VU used by handle to be returned. The fix is to have another context that is used for the actual runContext of the VU so we can cancel it when we get gracefully stopped. I tried to simplify (and speed up) the looping by only going through all the channels and context stuff if there was a change, otherwise we use a int32 and some atomic.LoadInt32 to see that "there is no change" and we directly start a new iteration. Additionally now the getVU error is propagetted by the vu handle's start, which is probably badly handled in this commit. Also the function that does the iteration now returns whether it was interrupted mostly to help with faster interruption of the fast looping. This might be reverted in a future commit, if the speed up isn't wanted. Unfortunately in order to support returning the VU and not returning it and still gettign the new VU but still to get start to actually return the error from getVU it took way more code then was anticipated ... The speed up is "insignficant" too, by my proffiling we were spending around 150-250ms (of 30s) in the mutex locking/channel reading now we spend 10ms(of 30s) in loading that int32. So 15-25x, but this is some sub percentage time and this is for an empty bodied `default` function. More benchmarks to come :D.
mstoykov
force-pushed
the
fixExternalExecutorNotPausing
branch
from
aeb022d
to
99bfe04
Compare
This is not the best of benchmarks :D name iterations/ns VUHandleIterations-8 0.04 ± 1% so around 40ns per iteration
There was a problem hiding this comment.
LGTM, or rather, I don't see any obvious issues, but the vuHandle code has gotten so complex that it's not obvious there aren't actually any issues... 😞
I like how the fast path is implemented, and I somewhat like that start() can return an error, if getting a VU fails. But my gut feeling is that either this can be simpler, or that we're doing something wrong if it can't be... In any case, I think simplification/refactoring this should be left as a low prio exercise for some future time, and we should use this version, unless we find a bug in it...
I'll create an refacotring low prio issue and reference this PR, with some ideas I had to maybe refactor/simplify the code, @mstoykov please chime in it if you have some of your own.
No description provided.