Fix the way we integrate libuv to NSApp #82

Closed
zcbenz opened this Issue Oct 10, 2012 · 22 comments

Projects

None yet

4 participants

@zcbenz

Currently we're simply polling uv events while running [NSApp nextEventMatchingMask:], we should replace it with monitoring file descriptors with CFRunLoopAddSource.

Note that libuv is removing libev in the v0.9 branch, so we need to wait until it's done, since uv's underlying work may largely change.

@rogerwang
nwjs member

I think I wrote the main loop with CFRunLoopAddSource in the past. Please find out why it's changed to polling.

If it hurt the performance pls fix it ASAP. We can't wait for upstream to transit from libev.

@zcbenz

The removal of libev has been done in libuv's rm-ev branch yesterday, I'll implement the message loop integration with libuv's new implementation before the release of v0.3.2.

@rogerwang
nwjs member

Please don't use that branch since it is still experimental, which could bring in problems.

And I don't think it would cost much effort to modify our code for the new libuv in the future -- they work in the same way basically.

@zcbenz

libuv doesn't implement everything with libev, timers and idlers are implemented with its own code, that's why previous message loop integration fails. I'll solve this issue with current stable libuv.

@zcbenz

In the new implementation, kqueue's backend fd is listened and we do uv_run in the callback. This makes most operations as fast as pure libuv message loop.

However kqueue is bugged on Mac, for some conditions, such as the first connection to socket created by listen, will not cause kqueue's backend fd to fire the callback. So there's still a polling every 500ms in case of such bugs. Node.js doesn't has this problem because it's always polling.

Because of the existence of polling, message loop's performance on Mac is not as good as on Windows and Linux, I tried to use node-tar to untar a 70mb file, nw's performance on Windows and Linux is as good as pure node process, however on Mac it is 50%~100% slower. After disabling polling, Mac is only 10%~20% slower, but some functions (like net.listen) will not work correctly because of kqueue's bug.

Close this because libuv may still use kqueue even after the removal of libev, so our implementation should be good enough.

@zcbenz zcbenz closed this Nov 6, 2012
@sequoiar

why not put libuv loop in a separate thread, at same time keep libuv/node.js in the same context as render process? polling libuv in render loop is bad thing.

@rogerwang
nwjs member

@sequoiar , mixing thread and dealing with context switch in callbacks and function calls is not as good as merging main loops. We've removed most of the polling by this commit, the performance is good now and we will continue to improve it.

@sequoiar

the current CPU always has multi-cores/threads, so there should not be a context switch, just one process owns two threads: render is main thread, libuv is another.

in case node.js/libuv run heavy networking service, many events coming to render process/thread, then it may slow down render event processing, that increase response time from user input.

@rogerwang
nwjs member

By "context" I mean the context in the Chromium/V8 stack, you'll have to post/delay the processing and wait for the other thread to pick up its work.

If you have any benchmark/test cases showing any problem, welcome to contribute.

@rogerwang
nwjs member

btw, which is off-topic, when you have a multi-core CPU doesn't mean you don't have much 'context switch'. Run 'vmstat 1' and find the 'cs' value.

@creationix

This fix made my OSX app much faster initially, but after I added in pty.js (a fork(3) binding), it got super slow. Every libuv event now has around 1000ms of latency. This is using 0.3.2.

I think we need to use the new uv_wait() to properly integrate. @indutny worked a lot on integrating with the OSX event loop using this.

@rogerwang rogerwang reopened this Nov 7, 2012
@rogerwang
nwjs member

Thanks for the info. We'll look into it. And help will be appreciated.

@rogerwang
nwjs member

@creationix , we tried to fix the problem in the same way as 'uv_wait()'. We tested it and the result looks better. You can have a try here: https://s3.amazonaws.com/node-webkit/playground/nw_release_mac.zip

@creationix

Much better, thanks! It performs about the same as linux as far as my eye can tell. Hopefully uv_wait() will land in libuv soon.

@creationix

Hmm, something is very broken in my app though. Many operations freeze up the app and require me to reboot the process.

@creationix

My app appears to die after an alert, confirm, or prompt dialog is used. Everything seems fine before that.

@zcbenz

alert and its friends created a nested message loop to deal with events, and they blocked the current execution of javascript context, which had caused problems for the uv_wait way of solving problems since it's poling all the time in another thread. I'm trying to fix it by making it happy with Chrome's nested message loop and pause the loop when necessary.

But I do not recommend using alert and other dialogs in real applications, since they will pause everything (including node.js functions), which is useful for debugging but would be a surprise for many people.

@zcbenz zcbenz added a commit that referenced this issue Nov 9, 2012
@zcbenz zcbenz Make node tests run again after showing an alert dialog.
This change is for #82.
f4f3bb7
@zcbenz

@creationix you can try our new fix at https://s3.amazonaws.com/node-webkit/playground/nw_release_mac.zip , it has passed our tests and should have fixed your problem, thanks for testing.

@creationix

In initial testing, this seems to work. The UI freezes while the dialog is up as expected. When the dialog is dismissed, my program continues to run normally.

Also, I'm not sure if it was clear, but I expect the libuv event loop to be blocked while the dialog is up. It would be very surprising for my http server to keep accepting requests when the main thread is blocked by the nested event loop.

@rogerwang
nwjs member

In the testing version of mac, we do block the libuv loop on dialog up (nested loop). We'll also ensure this behavior for Linux and Win. Will keep you posted.

@kevinsawicki kevinsawicki referenced this issue in atom-archive/terminal Jul 2, 2013
Closed

Heavy CPU usage while open #3

@rogerwang
nwjs member

turns out kqueue is not buggy for us because there is flaw in the libuv integration. The previous commit fixes delay in I/O and process.nextTick() widely seen.

@rogerwang
nwjs member

The event loop on Mac was rewritten to fix this.

@rogerwang rogerwang closed this Sep 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment