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

Default to using setTimeout instead of setImmediate #54

Open
hexonaut opened this issue Apr 1, 2015 · 11 comments
Open

Default to using setTimeout instead of setImmediate #54

hexonaut opened this issue Apr 1, 2015 · 11 comments

Comments

@hexonaut
Copy link

hexonaut commented Apr 1, 2015

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
Copy link
Owner

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
Copy link
Author

hexonaut commented Apr 1, 2015

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
Copy link

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
Copy link
Owner

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
Copy link
Owner

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

@francescoagati
Copy link
Contributor

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
Copy link

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

@francescoagati
Copy link
Contributor

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

@NoxHarmonium
Copy link

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: YuzuJS/setImmediate#35

@jdonaldson
Copy link
Owner

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
Copy link
Owner

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.

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

No branches or pull requests

4 participants