sofie-atem-connection icon indicating copy to clipboard operation
sofie-atem-connection copied to clipboard

feat: remove use of nanotimer

Open Julusian opened this issue 9 months ago • 2 comments

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

  • What is the current behavior? (You can also link to an open issue here)

nanotimer is used to ensure accurate timing in timers, at the cost of increased cpu usage.

@mint-dewit can you remember any justification for using nanotimer over regular nodejs timers? https://github.com/nrkno/sofie-atem-connection/commit/84482cad64bf5213eef1ce99eaaecc2e36762ae8 I can't find anything in asana or the github issues that give an explanation

  • What is the new behavior (if this is a feature change)?

use setTimeout instead of nanotimer.

In my testing, this gave accurate enough results. And as we run this timer in a worker thread, there shouldnt be much/any contention on cpu time within the thread. It is possible that the timer callback will be late if overall system cpu usage is high, but I am not sure if nanotimer would do this much better, as it will be compounding the cpu starvation with its spin loop, and it still relies on the event loop running to run the next iteration.

  • Other information: Testing shows setTimeout is able to pretty accurately fire after short periods. This will improve performance, as nanotimer spins the cpu to hit its target. Tested using a variation of https://github.com/nodejs/node/issues/26578#issuecomment-471392004

This could probably do with some more stress testing in a production like environment to ensure it doesnt cause any issues in a real environment.

Julusian avatar Sep 25 '23 11:09 Julusian

Codecov Report

Patch coverage: 95.45% and project coverage change: +0.01% :tada:

Comparison is base (a4f9b8b) 86.28% compared to head (e84cf95) 86.30%. Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
+ Coverage   86.28%   86.30%   +0.01%     
==========================================
  Files         180      180              
  Lines        5812     5819       +7     
  Branches      957      960       +3     
==========================================
+ Hits         5015     5022       +7     
  Misses        775      775              
  Partials       22       22              
Files Changed Coverage Δ
src/lib/atemSocketChild.ts 94.19% <95.45%> (+0.13%) :arrow_up:

... and 2 files with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Sep 25 '23 11:09 codecov-commenter

can you remember any justification for using nanotimer over regular nodejs timers?

My memory is probably incorrect given how long ago it was but I think the default timers were not always fast enough to send the acknowledgements. I think at that time the atem protocol was quite finicky and it was required to send the ack's within 5 ms or something. Being too slow would cause the entire connection to be thrown away by the atem. I think they've made the protocol quite a bit more tolerant to timing these days.

It's also possible node has just gotten more accurate with it's timers, after all 2018 is a long time ago.

mint-dewit avatar Oct 02 '23 08:10 mint-dewit