-
-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
👍 |
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) |
So you would say to use |
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). |
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 |
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 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. |
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 |
Sure, it's latency, not perf. Just trying to figure out when to use
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). |
Related: pouchdb/pouchdb#5645 |
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. |
by the way I was suggesting |
|
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 usessetTimeout
. So we should avoid that.