nrkno / sofie-atem-connection

Sofie ATEM Connection: A Part of the Sofie TV Studio Automation System
https://github.com/nrkno/Sofie-TV-automation/
MIT License
129 stars 36 forks source link

feat: remove use of nanotimer #154

Open Julusian opened 11 months ago

Julusian commented 11 months ago

feature

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

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.

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.

codecov-commenter commented 11 months ago

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 ```diff @@ 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](https://app.codecov.io/gh/nrkno/sofie-atem-connection/pull/154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nrkno) | Coverage Δ | | |---|---|---| | [src/lib/atemSocketChild.ts](https://app.codecov.io/gh/nrkno/sofie-atem-connection/pull/154?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nrkno#diff-c3JjL2xpYi9hdGVtU29ja2V0Q2hpbGQudHM=) | `94.19% <95.45%> (+0.13%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/nrkno/sofie-atem-connection/pull/154/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nrkno)

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

mint-dewit commented 11 months ago

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.