[go: nahoru, domu]

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

use immediate instead of setImmediate #52

Closed
wants to merge 1 commit into from
Closed

Conversation

nolanlawson
Copy link
Contributor
@nolanlawson nolanlawson commented Sep 7, 2016

Using setTimeout is slower than using immediate, which defaults to using MutationObserver to make microtasks. In the browser, I notice a lot of time is just spent idle because it uses setTimeout. So we should avoid that.

@nolanlawson nolanlawson changed the title Test in multiple Node versions and browsers use immediate instead of setImmediate Sep 7, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.13% when pulling c9b48fc on immediate into 8cf8870 on master.

@juliangruber
Copy link
Member

👍

@calvinmetcalf
Copy link
Contributor

I would suggest replacing only process.nextTick with immediate and keeping setImmediate as a first choice as immediate with block io during recursive calls (in fact the current implementation of process.nextTick is based on immediate so they are very similar except for initial latency of non recursive calls)

@nolanlawson
Copy link
Contributor Author

So you would say to use global.setImmediate || require('immediate')? Isn't this basically going to give one experience to IE/Edge and another experience to every other browser? Or should we just use setImmediate in Node but not the browser?

@nolanlawson
Copy link
Contributor Author

Also it's unclear to me why we should worry about the I/O blocking – I wouldn't expect MemDOWN to block any less than a regular synchronous in-memory database, which of course would block a lot. My primary desire for MemDOWN is just to be fast; if it blocks, then folks should move it to a web worker (maybe in Node though the concerns are different).

@calvinmetcalf
Copy link
Contributor

I don't know, did they fix the IE bug where setImmediate could bloke the event loop? if not then they'll ~ the same thing.

As for blocking the thing is if you do quite a lot of calls recursively, like write 100 things but one at a time and then make the next call in the call back then something truly async would allow other callbacks to fire between the async calls. Or put another way I'm suggesting you use a macrotask not a microtask

Also this is a discussion of latency not performance

@nolanlawson
Copy link
Contributor Author
nolanlawson commented Sep 7, 2016

You don't have to say "they;" the person who implemented setImmediate works like one floor up from me and I could just ask them. 😉

I tested out the demo page linked from that GitHub issue, and I'm not sure what I'm supposed to be seeing, but I see the same effect in Edge 14 desktop, Edge 14 mobile, and IE 11 desktop. I assume in the broken case that it would stop outputting logs? The test case is not super clear.

As for allowing other callbacks to interleave, my main concern is that process.nextTick() under the hood seems to contribute to a lot of idle time when running memdown in the browser, which we can eliminate by using microtasks. If you compare memdown to something like LokiJS (which is synchronous), then the overall runtime is larger because of the timeouts. (In fact right now the in-memory adapter for pouchdb-server is actually slower than CouchDB even though it doesn't write to disk, just because of all the idle time (I think)).

To be fair though I need to actually run benchmarks instead of just making wild claims. Maybe there's a better way to speed up memdown.

@calvinmetcalf
Copy link
Contributor

your benchmark is probably testing latency more then performance, if you parallelize it by running like running 1000 or 10000 at once you might get a more representative demo for a real world app where other things are happening at the same time

@nolanlawson
Copy link
Contributor Author

Sure, it's latency, not perf. Just trying to figure out when to use setTimeout and when to use setImmediate/immediate; it's really not clear to me. Yes microtasks block I/O, but so do literally every other thing in JavaScript except setTimeout/setImmediate, and in the browser setTimeout has basically a built-in 4ms latency cost. For memdown it seems super conservative to use setTimeout in the browser, but maybe I'm wrong.

if you parallelize it by running like running 1000 or 10000 at once you might get a more representative demo for a real world app where other things are happening at the same time

I'll try to come up with some numbers for PouchDB, but in the test suite alone I see a huge amount of idle time, because it doesn't parallelize everything (since it can't, since it's writing to disk).

@nolanlawson
Copy link
Contributor Author

Related: pouchdb/pouchdb#5645

@nolanlawson
Copy link
Contributor Author

Gonna defer to @calvinmetcalf's judgment here, at least until I can prove a big perf diff and that the I/O blocking is worth it.

@calvinmetcalf
Copy link
Contributor

by the way I was suggesting global.setImmediate || require("immediate"); instead of just require("immediate"); not scuttle it entirely

@nolanlawson
Copy link
Contributor Author

setImmediate makes sense for Node, but for the browser I worry about putting IE/Edge down different code paths from other browsers. This has actually come up a lot in my web compat work at Edge and it's surprising how often this can have a negative impact on Edge.

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.

None yet

4 participants