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

Already on GitHub? Sign in to your account

Windows Port #81

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

mojodna commented Feb 7, 2012

Moved IOWatcher functionality into C++ and adapted it for use on Windows (Vista+ due to use of WSAPoll). Everything appears to work as it should on both my Mac and in a Windows 7 VM, but I still won't fully trust my changes until others can observe the same.

I've included rough Windows build instructions, but it needs to be better packaged up to be a) usable and b) distributable. If you know of resources for creating cross-platform NPM packages containing binary add-ons, please post them here.

Addresses #74, maybe.

@tj tj and 1 other commented on an outdated diff Feb 7, 2012

@@ -248,10 +276,59 @@ enum {
return args.This();
}
+ bool
+ Socket::IsReady() {
+ size_t len = sizeof(int);
+ struct pollfd pfd = { 0 };
+ pfd.events = POLLIN;
+
+ if (socket_ && zmq_getsockopt(socket_, ZMQ_FD, &pfd.fd, &len) >= 0) {
+ return sockpoll(&pfd, 1, 0);
+ } else {
+ return 0;
+ }
@tj

tj Feb 7, 2012

Collaborator

will this work on windows for you?:

  bool
  Socket::IsReady() {
    zmq_pollitem_t items[1];
    items[0].socket = socket_;
    items[0].events = ZMQ_POLLIN;
    return zmq_poll(items, 1, 0);
  }
@mojodna

mojodna Feb 7, 2012

Contributor

Hard to tell. It looked like it worked once (based on some debug output). Windows doesn't seem to be doing the right thing with return codes (under bash or cmd.exe), so the tests aren't especially helpful at the moment (they always report success unless something throws). I guess I know what I'm looking at next.

I agree that this would be better; not only does it remove the Vista+ restriction, it's more likely to work on other systems with different poll implementations.

@tj

tj Feb 7, 2012

Collaborator

yup zmq already implements kqueue and friends so we should utilize that. as for the test suite yeah we can ditch bash and go with a little node script that loops through and does the same thing

@mojodna

mojodna Feb 7, 2012

Contributor

How 'bout a proper test runner? Say...mocha or something.

@tj

tj Feb 7, 2012

Collaborator

nah we wouldn't gain much from a test framework for this sort of thing, the tests are so large it's a lot nicer to separate these ones into separate files. We could if we have a decent reason to but other than the small set of unit tests I dont think we really need it

@mojodna

mojodna Feb 7, 2012

Contributor

They were finishing immediately because I thought I was being clever and uv_unref'ing after registering the uv_check. With that fixed, the tests legitimately pass on my Mac (with your simplified IsReady() implementation).

However, uv_check_start / uv_check_initdon't appear to work in any meaningful way on Windows. I'm very unfamiliar with libuv and node internals, so I could be doing something wrong...

Anyway, the upshot is that this works fine on OS X (not sure about performance differential), but isn't yet ready for Windows.

@tj tj and 1 other commented on an outdated diff Feb 7, 2012

binding.vcxproj
@@ -0,0 +1,89 @@
+<?xml version="1.0" encoding="utf-8"?>
@tj

tj Feb 7, 2012

Collaborator

.gitignore :D

@mojodna

mojodna Feb 7, 2012

Contributor

Until we can get gyp (or something) to generate these, this and binding.sln (the "solution" file) stay. If it makes you feel better, think of them as Makefiles for Windows.

@tj

tj Feb 7, 2012

Collaborator

ah ok sounds good then for now

Collaborator

tj commented Feb 7, 2012

this also takes care of #79

Contributor

mojodna commented Feb 7, 2012

Doesn't work on Windows yet, possibly due to uv_check_start and uv_check_init.

Collaborator

tj commented Feb 7, 2012

added make benchmark: #82 so we have something to reference at least for now.

Contributor

mojodna commented Feb 8, 2012

A relative comparison:

IOWatcher:

  pub/sub:
    100000 msgs
    16202 ops/s
    6172 ms

native:

  pub/sub:
    100000 msgs
    16374 ops/s
    6107 ms
Collaborator

tj commented Feb 8, 2012

cool, well at least there's no regression, on my air im getting:

  pub/sub:
    100000 msgs
    22426 ops/s
    4459 ms

not a great benchmark though we should spawn a few subprocs so we're not grinding the one loop

Contributor

mojodna commented Feb 13, 2012

joyent/node#2738 has details on why uv_check_* isn't the right fit for this (but they're not clear on what is yet). Their explanation could also be the reason why perf is substantially lower than czmq (I seem to remember IOWatcher using ev_check_*, but I could be wrong).

caili commented Mar 9, 2012

Following the advise to use uv_timer_* (https://github.com/caili/zeromq.node/tree/windows), though tests run fine but it hangs occasionally in one of my projects. I tried with polling in js (https://github.com/caili/zeromq.node/tree/polling). Hardly a solution but it works so far for me in a prototype.

Contributor

mojodna commented Mar 9, 2012

See also https://github.com/matthiasg/zeromq-node-windows

Matthias says that he's getting 14-15k messages/second on Windows with his changes.

caili commented Mar 9, 2012

With Matthias's changes the node hangs in my environment (XP / VS 2010 / Node 0.6.10, a couple of REQ/REP and PUB/SUB sockets running in parallel). Basic REQ/REP or PUB/SUB tests run fine though.

@caili did you create new tests that I could use to recreate the hangs on my machine ? Also I thought @mojodna made changes that would require vista and up ?

btw i used node 0.6.12 and 0.6.13 is even better, since creating the csproj works correctly now

Contributor

mojodna commented Mar 16, 2012

Also I thought @mojodna made changes that would require vista and up ?

By switching to zmq_poll, that requirement went away (it was only when using WSAPoll).

caili commented Mar 16, 2012

@matthiasg all tests I made pass. I guess this is something else in the application I have to figure out.

caili commented Mar 19, 2012

turned out to be an application bug. The uv_timer_* approach works pretty well :-)

@mojodna mojodna commented on the diff Jun 25, 2012

binding.cc
@@ -248,10 +260,57 @@ enum {
return args.This();
}
+ bool
+ Socket::IsReady() {
+ zmq_pollitem_t items[1];
+ items[0].socket = socket_;
+ items[0].events = ZMQ_POLLIN;
+ return zmq_poll(items, 1, 0);
+ }
+
+ void
+ Socket::CallbackIfReady() {
+ if (this->IsReady()) {
@mojodna

mojodna Jun 25, 2012

Contributor

This may be unnecessary since the edge transition on the socket already told us that it's ready.

@mojodna mojodna commented on the diff Jun 25, 2012

lib/index.js
@@ -4,8 +4,7 @@
*/
var EventEmitter = require('events').EventEmitter
- , IOWatcher = process.binding('io_watcher').IOWatcher
- , zmq = require('../build/Release/binding');
+ , zmq = require('../binding');
@mojodna

mojodna Jun 25, 2012

Contributor

If you see "Symbol not found" errors, either change this path back or copy the built version over ./binding.node.

Contributor

mojodna commented Jun 25, 2012

Closing in favor of #110 (uv_poll--this isn't just a Windows thing), which contains the commits I just pushed.

@mojodna mojodna closed this Jun 25, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment