Skip to content

Conversation

@aduth
Copy link

@aduth aduth commented Nov 7, 2019

It doesn't seem I can easily edit or fork the referenced benchmark, but in the latest Chrome (78), this appears to perform slightly better than the current implementation:

image

Benchmark details Setup:
const store7 = createStore();
let viaPreallocated = (function viaPreallocated(store, action) {
    return function() {
        let args = new Array(arguments.length + 1);
        args[0] = store.getState();
        for (let i=0; i<arguments.length; i++) args[i+1] = arguments[i];
        return a7(args);
    };
})(store7, 'viaPreallocated');

Test:

viaPreallocated('a');
viaPreallocated('b');
viaPreallocated(1, 2);
viaPreallocated({});

It's a technique we use in our state library's selector binding.

The size bump is bigger than expected, not significant, but enough to fail tests without adjusting the permitted bundle size.

@aduth
Copy link
Author

aduth commented Nov 12, 2019

I managed to reclaim a few bytes in 5f4fbdb by switching to a while loop.

  full/preact.js dist/unistore.js preact.js
master 760b 355b 546b
eeec990 777b 373b 546b
5f4fbdb 773b 366b 546b

I tried a few other ideas (unsuccessfully) as well:

  • Trying to attain the optimization from length assignment while avoiding explicit assignment of state.
let args = [ state ];
args.length = arguments.length;

This regressed on the benchmarked gains. I assume it's because the benefit comes from the constructor of an array with length.

  • Moving ret assignment to the top of the function.

Interestingly, this uses 2 fewer characters before gzip, but ultimately results in a larger gzipped bundle size (diff).

@lkmill
Copy link

lkmill commented Dec 27, 2019

@aduth hello stranger. i opened a couple of pull requests with rather substantial changes that i personally think greatly improves unistore and was looking for some feedback on. i'm assuming @developit is busy and meanwhile i was hoping to get this feedback from elsewhere. since you have opened a pull request yourself i assume you are familiar with the package and would love to hear what you think about the changes i propose. you can find them at #182 and #183. the second one builds upon the changes from the first.

@aduth aduth closed this by deleting the head repository Dec 26, 2022
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.

2 participants