jdonaldson / promhx

A promise and functional reactive programming library for Haxe
MIT License
145 stars 24 forks source link

Default to using setTimeout instead of setImmediate #54

Open hexonaut opened 9 years ago

hexonaut commented 9 years ago

The setImmediate polyfill is riddled with bad code. Running a performance test of a game I am building saw an overhead of a whopping 35% overhead due to the functions attachTo.setImmediate and tasks.runIfPresent. They both cannot be optimized by the V8 compiler and seem to be used heavily for promhx.

Simply turning on the noEmbedSetImmediate flag I saw a huge increase in performance with setTimeout only using 3.84% of the CPU.

From my understanding the setImmediate is being used in promhx to improve performance. Since it seems that browsers other than IE do not intend to implement setImmediate (https://developer.mozilla.org/en-US/docs/Web/API/Window/setImmediate) and that the polyfill is terrible for performance, I would recommend either using setTimeout by default or even axing setImmediate altogether.

jdonaldson commented 9 years ago

Wow, is this really true? 100% of the reason I was using setImmediate polyfills was because they were absolutely destroying the setTimeout based version.

When you say "performance" are we talking about throughput ("thens" per second), or just cpu utilization? That's one explanation I could see for this.

hexonaut commented 9 years ago

I am talking about taking a CPU profile on the chrome dev tools. The overhead from the polyfill may go away if the functions are optimized, but I did not test this.

NoxHarmonium commented 9 years ago

I am experiencing this also. I am working on a Flambe HTML5 game with a very heavy use of promises and streams. I have only tested on Chrome but when setImmediate is used the throughput is so bad that there was noticeable lag in the UI (when hovering over buttons etc.) and the time waiting for promises to resolve actually takes longer than the actual work done to resolve them. After seeing this issue thread I set the -D noEmbedSetImmediate and the speed up is incredible. I was working on a fork to allow disabling the event loop to make the game more responsive but I may not need to anymore.

jdonaldson commented 9 years ago

Ok, I'm seeing it now too... it looks like somehow the situation with setImmediate has reversed completely!

I bumped the version of setImmediate, and it got rid of the terrible performance, but setTimeout is still faster. I'll dig a bit further, but will likely make a change soon.

jdonaldson commented 9 years ago

I'm marking this as an enhancement, pending a v2 release.

francescoagati commented 9 years ago

i can confirm. using setTimeout with explorer when there are mani events compressed(like mouseover or touchmove) is more performant that setImmediate. Is possible to have a flag for using setTimeout instad of setImmediate.

something like


#elseif (js && (noEmbedJs || useSetTimeout) && !nodejs)
            // fallback to setTimeout
            untyped __js__("(setTimeout)")(f);
NoxHarmonium commented 9 years ago

@francescoagati There is a compiler flag already to use setTimeout. Just pass -D noEmbedSetImmediate to the compiler at build time.

francescoagati commented 9 years ago

Yes bu notembedsetimmediate use the setimmediate of the browser. I want that eventloop run only in setTimeout

NoxHarmonium commented 9 years ago

Yeah it looks like using setImmediate in IE10+ has a bug which affects performance. If IE is the only browser that supports setImmediate and it doesn't even handle it properly it might be a good idea to remove it from the fallback.

See also: https://github.com/YuzuJS/setImmediate/issues/35

jdonaldson commented 9 years ago

Thanks for the notes here, I hadn't seen that IE10+ bug.

FYI, you can override the setImmediate polyfill by just setting the EventLoop to setTimeout manually. At that point, it makes sense to just use the -D noEmbedSetImmediate as well to prevent it from getting included in the output.

jdonaldson commented 8 years ago

FYI, I'm using setImmediate by default in node still, but I'm no longer including the setImmediate polyfill in browser based versions by default. Accordingly, I've dropped noEmbedSetImmediate and use embedSetImmediate as the new -D flag.