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

Add MessageChannel implementation to improve performance #4

Open
TestPolygon opened this issue May 27, 2020 · 6 comments
Open

Add MessageChannel implementation to improve performance #4

TestPolygon opened this issue May 27, 2020 · 6 comments

Comments

@TestPolygon
Copy link

The description says that it is "Simple setImmediate module".
And people use it (over 2kk installations). They think that it's simple and good one, but in fact it's simple and slow one. But there is no any note about it. A lot of people do not check the code of modules that they install, they expect that the creator spent time to investigate the problem to create the best decision, but it is not about this repo. The word "simple" is not the correct description, but "simple and naive", or "simple and slow" are.

Using this code in the browser leads to a performance hit. It's a fact. For some cases the impact is significant.
For example: Stuk/jszip#617

Here is the example of using of the properly created setImmediate:
https://jphpsf.github.io/setImmediate-shim-demo/
296ms (setImmediate) vs 12617ms (setTimeout === your implementation)

Node.js has setImmediate. Using your implementation in the browser is the same thing as using setTimeout, but with the misleading name.

@sindresorhus
Copy link
Owner

You're right. My use-case for making this was not in hot code, so the setInterval overhead didn't matter. When I made this, the shims were all bloated and buggy. What is currently the best shim? I'm happy to recommend that instead.

@TestPolygon
Copy link
Author

TestPolygon commented Jun 8, 2020

Here is the examples of setImmediate implementation:
[1] https://github.com/YuzuJS/setImmediate/blob/master/setImmediate.js
and from core-js:
[2] https://github.com/zloirock/core-js/blob/master/packages/core-js/internals/task.js

I think it would be enough if you just add the implementation via MessageChannel (it will work in WebWorkers too (in comparison with postMessage) for the browsers (with setTimeous as a fallback).
It has the good browser support: https://caniuse.com/#search=MessageChannel

In this case your implementation will be really simple (in comparison with others) and work well in most of cases.

Also: Stuk/jszip#619 (comment)

@sindresorhus sindresorhus changed the title Naive and slow implementation Add MessageChannel implementation to improve performance Jun 9, 2020
@sindresorhus
Copy link
Owner

I think it would be enough if you just add the implementation via MessageChannel (it will work in WebWorkers too (in comparison with postMessage) for the browsers (with setTimeous as a fallback).

I agree. That's a good plan. I don't have time to work on this right now. Contributions are of course welcome. In the meantime, I added a note to the readme.

@AlttiRi
Copy link

AlttiRi commented Jun 22, 2021

Still not fixed?
Okay...

export const setImmediate = /*#__PURE__*/ (function() {
    const {port1, port2} = new MessageChannel();
    const queue = [];

    port1.onmessage = function() {
        const callback = queue.shift();
        callback();
    };

    return function(callback) {
        port2.postMessage(null);
        queue.push(callback);
    };
})();

This implementation is only for browsers, to use it in Node.js it requires to call ref/unref for the proper process exit, but Node.js already has setImmediate, so in Node use the native setImmediate.

UPD. For me it works fine, but it probably requires to add ...args as it is in the current implementation.
UPD2. Technically it requires also to add clearImmediate and to return immediateID, but the current implementation also does not have it.

@AlttiRi
Copy link

AlttiRi commented Jun 22, 2021

I don't know why @Stuk still not changed setImmediate library to an other one* while a lot of people ask him to do it. Okay, the other way to fix JSZip library is to fix this library if he do not want to use any other.

*Since the performance is terrible. It works up to 50 times slower because of this library.

@AlttiRi
Copy link

AlttiRi commented Jun 22, 2021

I'm sorry, but why not?

"use strict";
module.exports = /*#__PURE__*/ (function() {
    require("setimmediate");
    return setImmediate;
}());

It's better to do a wrapper for the other properly working library, than misname setTimeout with setImmediate for millions of people.

People specially use this library to get setImmediate instead of just using of setTimeout, but they get setTimeout anyway. I think it's not OK, it's wrong.

In any case "setimmediate" library adds setImmediate function to the global scope since it's a polyfill, but that wrapper is just a module. So it makes sense to have it.


UPD.
Oops, it's not working (as a module).

It should be OK:

"use strict";
module.exports = typeof setImmediate === "function" ? setImmediate : require("./setImmediate-edited.js").setImmediate;

But it requires to modify the original file a bit:

Replace this:

    attachTo.setImmediate = setImmediate;
    attachTo.clearImmediate = clearImmediate;

with

    module.exports = {setImmediate, clearImmediate};

Optionally remove that (as a useless code):

    // If supported, we should attach to the prototype of global, since that is where setTimeout et al. live.
    var attachTo = Object.getPrototypeOf && Object.getPrototypeOf(global);
    attachTo = attachTo && attachTo.setTimeout ? attachTo : global;

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

3 participants