Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remove use of nanotimer #154

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat: remove use of nanotimer #154

wants to merge 2 commits into from

Conversation

Julusian
Copy link
Member

  • 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? 84482ca 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.

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.

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 nodejs/node#26578 (comment)
…96

This is so that it only runs when packets are inflight, and lowers idle cpu usage
@codecov-commenter
Copy link

Codecov Report

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

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%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mint-dewit
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants