nodejs / hardware

Hardware Working Group
42 stars 16 forks source link

Switch Raspi-IO to using node-ffi #11

Closed nebrius closed 9 years ago

nebrius commented 9 years ago

Now that we know there is an async version of node-ffi from https://github.com/nodejs/hardware/issues/9 (thanks @TooTallNate!), let's get it in production to see how well it works for robots.

I'll convert Raspi-IO first, since it's a native module I oversee and isn't nearly as popular or depended on as node-serialport, but still has a following.

Once this is done, we can wait a while and see if there are any issues that crop up and then we can look at converting others, if all goes well.

voodootikigod commented 9 years ago

Can you keep notes on the process so we can port node-sp in the same fashion? <3z

Chris Williams

@voodootikigod http://twitter.com/voodootikigod | GitHub http://github.com/voodootikigod

The things I make that you should check out: SaferAging http://www.saferaging.com/ | JSConf http://jsconf.com/ | RobotsConf http://robotsconf.com/ | RobotsWeekly http://robotsweekly.com/

Help me end the negativity on the internet, share this http://jsconf.eu/2011/an_end_to_negativity.html.

On Mon, Jun 1, 2015 at 1:49 PM, Bryan Hughes notifications@github.com wrote:

Now that we know there is an async version of node-ffi from #9 https://github.com/nodejs/hardware/issues/9 (thanks @TooTallNate https://github.com/TooTallNate!), let's get it in production to see how well it works for robots.

I'll convert Raspi-IO first, since it's a native module I oversee and isn't nearly as popular or depended on as node-serialport, but still has a following.

Once this is done, we can wait a while and see if there are any issues that crop up and then we can look at converting others, if all goes well.

— Reply to this email directly or view it on GitHub https://github.com/nodejs/hardware/issues/11.

nebrius commented 9 years ago

@voodootikigod will do!

nebrius commented 9 years ago

I have some preliminary results. Raspi-IO is composed of a bag of modules I wrote, and I just finished converting one of them over: raspi-gpio.

This one only had synchronous calls, e.g. doing all the work in NAN_METHOD(foo) {} directly as opposed to instantiating a NanAsyncWorker. This was way easy, as it turns out.

I basically copied the C++ code inside the NAN_METHOD calls into JavaScript, undid the parameter marshalling, and hand-transpiled the rest. It took me about 20 minutes to do, but then again this is a small, simple module. You can see the diff here: https://github.com/bryan-m-hughes/raspi-gpio/commit/92b4d95d39ac1bf04a851cbbaa435a8d3962ad26

nebrius commented 9 years ago

I'm going to tackle the raspi module next, which does make use of a NanAsyncWorker

fivdi commented 9 years ago

@bryan-m-hughes have you got any information about the performance of raspi-gpio before and after the modification? For example, the number of calls per second possible for DigitalInput#read and DigitalOutput#write?

voodootikigod commented 9 years ago

Maybe I am not understanding this , but isn't node-ffi ALSO a native module and therefore aren't we just consolidating pain (not a bad thing, just a question)?

Chris Williams

@voodootikigod http://twitter.com/voodootikigod | GitHub http://github.com/voodootikigod

The things I make that you should check out: SaferAging http://www.saferaging.com/ | JSConf http://jsconf.com/ | RobotsConf http://robotsconf.com/ | RobotsWeekly http://robotsweekly.com/

Help me end the negativity on the internet, share this http://jsconf.eu/2011/an_end_to_negativity.html.

On Tue, Jun 2, 2015 at 8:33 AM, Brian Cooke notifications@github.com wrote:

@bryan-m-hughes https://github.com/bryan-m-hughes have you got any information about the performance of raspi-gpio before and after the modification? For example, the number of calls per second possible for DigitalInput#read and DigitalOutput#read?

— Reply to this email directly or view it on GitHub https://github.com/nodejs/hardware/issues/11#issuecomment-107938299.

fivdi commented 9 years ago

At the moment, yes, but node-ffi may (will?) end up in core. See here.

nebrius commented 9 years ago

@fivdi I don't know performance yet, that's a great question (and idea)! I'll write up a benchmark and compare before and after. Once I get raspi ported, I'll try and come up with something similar for async calls too.

@voodootikigod I was also thinking that we might be able to make node-ffi a priviledged module that is, perhaps, built alongside maybe node.js or npm in their build systems, making node-pre-gyp a more viable solution. This way, we can cover older versions of node as well even on more esoteric platforms like the Raspberry Pi, making that install process less painful too. @TooTallNate, what do you think?

nebrius commented 9 years ago

There's a bigger performance gap than I expected :(

Here's my test code:

var raspi = require('raspi');
//var gpio = require('raspi-gpio.native');
var gpio = require('raspi-gpio.ffi');

var ITERATIONS = 1000;

raspi.init(function() {
  var input = new gpio.DigitalInput('GPIO4');
  var output = new gpio.DigitalOutput('GPIO17');
  var start, end;

  start = Date.now();
  for (i = 0; i < ITERATIONS; i++) {
    input.read();
  }
  end = Date.now();
  console.log('Read took ' + (end - start) + ' ms');
});

The FFI version of raspi-gpio (v1.3.0) averaged 360ms, whereas the native version of raspi-gpio (1.2.1) averaged 5ms. This was averaged across 5 runs each.

TooTallNate commented 9 years ago

I was also thinking that we might be able to make node-ffi a priviledged module that is, perhaps, built alongside maybe node.js

I'm trying to make this happen. See: https://github.com/nodejs/io.js/labels/ffi

voodootikigod commented 9 years ago

i think you were missing the native version time :)

Chris Williams

@voodootikigod http://twitter.com/voodootikigod | GitHub http://github.com/voodootikigod

The things I make that you should check out: SaferAging http://www.saferaging.com/ | JSConf http://jsconf.com/ | RobotsConf http://robotsconf.com/ | RobotsWeekly http://robotsweekly.com/

Help me end the negativity on the internet, share this http://jsconf.eu/2011/an_end_to_negativity.html.

On Tue, Jun 2, 2015 at 12:38 PM, Bryan Hughes notifications@github.com wrote:

There's a bigger performance gap than I expected :(

Here's my test code:

var raspi = require('raspi');//var gpio = require('raspi-gpio.native');var gpio = require('raspi-gpio.ffi'); var ITERATIONS = 1000;

raspi.init(function() { var input = new gpio.DigitalInput('GPIO4'); var output = new gpio.DigitalOutput('GPIO17'); var start, end;

start = Date.now(); for (i = 0; i < ITERATIONS; i++) { input.read(); } end = Date.now(); console.log('Read took ' + (end - start) + ' ms'); });

The FFI version of raspi-gpio (v1.3.0) averaged 360ms, whereas the native version of raspi-gpio (1.2.1). This was averaged across 5 runs each.

— Reply to this email directly or view it on GitHub https://github.com/nodejs/hardware/issues/11#issuecomment-108009117.

nebrius commented 9 years ago

@voodootikigod I think my brain kinda wondered off a bit there while posting :). I edited the comment, but of course there is no email notifications.

5ms for native, compared with 360ms for FFI, so about two orders of magnitude. These are synthetic tests of course, designed to measure raw performance, so the performance drop may not matter, depending on the circumstances.

fivdi commented 9 years ago

Using process.hrtime() rather than Date.now() should give more accurate measurements, but the trend isn't going to change. @bryan-m-hughes are you going to go ahead and switch everything in raspi-io over to ffi? I guess it's a tough decision.

nebrius commented 9 years ago

@fivdi right, I always forget about process.hrtime() :(

I'm leaning towards switching raspi-gpio back, but I'm not going to formally make a decision until later this week. I kinda want to let info this marinate a while.

Fortunately, raspi-io's version locking saves the day yet again, it's still on 1.2.1 of raspi-gpio. Seriously, that was the best decision I think I've made for raspi-io, even though I didn't know most of the reasons why at the time.

reconbot commented 9 years ago

Why it's slower is a big question, maybe it can improved upon? On Jun 2, 2015 3:48 PM, "Bryan Hughes" notifications@github.com wrote:

@fivdi https://github.com/fivdi right, I always forget about process.hrtime() :(

I'm leaning towards switching raspi-gpio back, but I'm not going to formally make a decision until later this week. I kinda want to let info this marinate a while.

Fortunately, raspi-io's version locking saves the day yet again, it's still on 1.2.1 of raspi-gpio https://github.com/bryan-m-hughes/raspi-io/blob/master/package.json#L20. Seriously, that was the best decision I think I've made for raspi-io, even though I didn't know most of the reasons why at the time.

— Reply to this email directly or view it on GitHub https://github.com/nodejs/hardware/issues/11#issuecomment-108075508.

nebrius commented 9 years ago

@reconbot I'll admit to not being very familiar with FFI and performance, @TooTallNate, perhaps you could weigh in if you have a free moment?

I do remember from a previous life not in JavaScript that these kinds of approaches are typically slower. IIRC, reflection in Java is usually about two orders of magnitude slower, and reflection in C# starts out about one order of magnitude slower and gets worse the more arguments are passed to the method.

fivdi commented 9 years ago

The performance figures observed correspond with what the Call Overhead section of the ffi readme says.

nebrius commented 9 years ago

After some consideration, I've decided not to switch the raspi-io ecosystem to FFI. The performance overhead is too great given that node.js at idle takes a high amount of resources as it is on the single core, ~600MHz Raspberry Pi 1.

EDIT: just for clarification, I reverted the changes in raspi-gpio and published v1.4.0 with these changes