Remove dependency on Immutable?

marcins commented 7 years ago

Would you be open to a PR that removes the dependency on ImmutableJS in favour of something lighter? I was thinking something like Icepick.

We are not using Immutable in the rest of our application with bundle size being one of the considerations that steered us away from it, so it would be nice if we didn't have to take on that additional weight to use redux-optimistic-ui.

At a quick glance it doesn't appear that there would be anything you're doing with Immutable that doesn't have an analog in Icepick.

mattkrick commented 7 years ago

the reason immutable is so heavy is because it implements some really crazy data structures that reduce the lookup times & GC made by immutable objects.

that said, V8 has had a few improvements on this front since immutable was released. I'd be open for a pure JS implementation if you wanted to do that. it'd be fun to benchmark the difference in GC.

marcins commented 7 years ago

What do you think would be a good benchmark? Just run some large number of actions that are a combination of optimistic and non-optimistic updates against subset of a large state?

Something like Benchmark.js would cover runtime benchmarking, but doesn't do memory / GC benchmarking (not sure if that's even possible to automate at this stage?)

I'll give it a whirl, see what happens! :)

mattkrick commented 7 years ago

For a decent benchmark

use optimistic(reducer, {maxHistory: 10000000}) (this will stop the memory leak messages)
each object is a new 1KB object
40% of the objects are not optimistic
30% of the objects are optimistic and succeed in 10ms
20% of the objects are optimistic and succeed in 1000ms (this will cause a history queue to form)
10% of the objects are optimistic and fail (this will hopefully cause all hell to break loose with the constant rollbacks & replays)
preferably deterministic otherwise it's just a monte carlo analysis (easy way to do it is a big 1e7 for-loop, and the action is determined by the modulo)

benchmark works fine, but i think the V8 profiler could give you some really interesting info wrt garbage collection. If the facebook guys are correct, we'll see massive spikes with the vanilla implementation & not much with immutable. If they're wrong, then we get to politely blog about it & make fun of them for making a bloated library that doesn't live up to the hype.

marcins commented 7 years ago

I created an initial rough implementation of the benchmarking, and tried to run it for 100K iterations, but it turns out calling setTimeout some 60,000 times might not be the most efficient way to do this - it takes a long time to complete :P

I haven't got the profiling / GC code working properly yet, but the benchmark on master completed in 23 minutes 30 seconds and my icepick branch in 22 min 55 seconds. So in terms of raw perf at least at a glance it seems they're comparable.

FWIW here's what the main loop currently looks like - I'm not sure if that's the kind of thing you had in mind?

for (let i = 0; i < SAMPLE_SIZE; i++) {
    let action;
    const select = i % 10;

    if (select <= 3) { // 40% are not optimistic
        store.dispatch(actionCreator(i));
        continue;
    }

    store.dispatch(actionCreator(i, optimist.BEGIN));
    if (select <= 6) { // 30% commit in 10ms
        setTimeout(() => store.dispatch(actionCreator(i, optimist.COMMIT)), 10);
    } else if(select <= 8) { // 20% commit in 1000ms
        setTimeout(() => store.dispatch(actionCreator(i, optimist.COMMIT)), 1000);
    } else { // 10% revert after 250ms
        setTimeout(() => store.dispatch(actionCreator(i, optimist.REVERT)), 250);
    }

    if (i % 1000 === 0) {
        console.log(`Running ${i} of ${SAMPLE_SIZE}`);
    }
}

Like I said, I'll keep working on it and hopefully get some useful info (I've been looking at v8-profiler and gc-stats as options for getting additional data.

marcins commented 7 years ago

I modified the benchmarking script to run in a slightly more realistic way. I have pushed up a repo at https://github.com/marcins/redux-optimistic-ui-benchmark with this script and other utils so that my methodology can be checked.

Initial results with 100,000 iterations show that:

the runtime for both options is pretty close (Immutable is slightly faster than Icepick)
Immutable uses more memory overall, but they have a similar number of GC events (with a similar amount of memory reclaimed each time)
Immutable has more consistent GC behaviour and memory usage over time. Icepick has more frequent smaller GCs at the beginning (this seems to be consistent between runs), and Immutable has a weird phase near the end of the run where it'll have GC half as often but recovering twice the memory.

Comparison of heap usage over time - it's a sawtooth because for each GC I'm plotting the before and after heap size at the same "time" on the X axis:

benchmark

mattkrick commented 7 years ago

Oh that's very good, nice work! I'm really surprised that the win in time is so marginal for immutable. Since icepick uses Object.freeze, which is notoriously slow, I bet a vanilla JS implementation would win out on both dimensions.

On Mar 7, 2017 5:00 PM, "Marcin Szczepanski" notifications@github.com wrote:

I modified the benchmarking script to run in a slightly more realistic way. I have pushed up a repo at https://github.com/marcins/ redux-optimistic-ui-benchmark with this script and other utils so that my methodology can be checked.

Initial results with 100,000 iterations show that:

the runtime for both options is pretty close (Immutable is slightly faster than Icepick)

Immutable uses more memory overall, but they have a similar number of GC events (with a similar amount of memory reclaimed each time)

Immutable has more consistent GC behaviour and memory usage over time. Icepick has more frequent smaller GCs at the beginning (this seems to be consistent between runs), and Immutable has a weird phase near the end of the run where it'll have GC half as often but recovering twice the memory.

Comparison of heap usage over time - it's a sawtooth because for each GC I'm plotting the before and after heap size at the same "time" on the X axis:

[image: benchmark] https://cloud.githubusercontent.com/assets/340441/23685104/789d1150-03f6-11e7-9e95-5b678a46031c.png

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mattkrick/redux-optimistic-ui/issues/21#issuecomment-284912083, or mute the thread https://github.com/notifications/unsubscribe-auth/AFQjv-lRly2Vxp_k589Bydmcc-8twZs6ks5rjf3FgaJpZM4MJZEm .

marcins commented 7 years ago

Icepick doesn't use Object.freeze in production, just in dev because of the perf overhead - these benchmarks are run with NODE_ENV=production (I think Immutable also has some prod specific optimisations?)

I made some small changes to the benchmark script to reduce some of the overhead of the benchmarking process. Didn't make much of a difference to the general data, but there were slightly fewer GCs and the two implementations finished a bit closer together:

benchmark

bhj commented 7 years ago

It would be cool if one could supply their own freezing method; there seem to be a number of immutable.js alternatives out there now!

mattkrick commented 7 years ago

yeah, too many. i'm not sure if that's necessary though. freezers are good for keeping you from doing something stupid (and in the case of immutable, to speed up huge lookups). for example, i'd rather make it a policy to use a freezer instead of having a user find a bug & sending a team of developers on a 4 hour hunt for figuring out where an object was mutated. This should not be an issue for your node modules because they are presumably more thoroughly tested than your application code. All kinda goes back to the same question: "is it still functional programming if the variable is mutated internally?" I say yes. So if we can guarantee that this package doesn't mutate your state object, then there's no need for a freezer in it (except for the immutablejs exception because I thought it would server to speed up the lookups).

bhj commented 7 years ago

@mattkrick The only downside of immutable.js seems to be size; I get about 160KB more JS (granted that is unminified/compressed and using Webpack 1). I'd just be +1 on reducing the heft if possible.

marcins commented 7 years ago

FWIW, here's the branch where I switched Immutable out for icepick: https://github.com/marcins/redux-optimistic-ui/tree/immutable-to-icepick. Would need a bit of cleaning up, there's some stray code from my initial benchmark experiments that has ended up in there too.

On 15 March 2017 at 05:07, Brandon Jones notifications@github.com wrote:

@mattkrick https://github.com/mattkrick The only downside of immutable.js seems to be size; I get about 160KB more JS (granted that is unminified/compressed and using Webpack 1). I'd just be +1 on reducing the heft if possible.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mattkrick/redux-optimistic-ui/issues/21#issuecomment-286510041, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUx2Ya5pO-0pQztnaye1tp8EyDyog4xks5rltdRgaJpZM4MJZEm .

-- M.

mattkrick commented 7 years ago

i'd be up for a PR for a vanilla JS implementation if anyone wants to take it on!

marcins commented 7 years ago

I finally had another look at this - I'm not convinced that a vanilla JS implementation would be much better than using icepick. Icepick is already relatively small (~2KB gzip?), doesn't have any dependencies, and works with plain JS objects.

To make redux-optimistic-ui have a vanilla JS implementation you'd need to implement immutable versions of things like slice, shift, set - why not just use the already tested ones in icepick?

There was a concern above with object freezing, but as mentioned that's only active when NODE_ENV=development - in production mode it's unfrozen JS primitives all the way.

I'm more than happy to open a PR with the icepick implementation - there's only a little bit of cleanup to do.

mattkrick commented 7 years ago

i'd argue that if someone doesn't want immutable, they probably don't want icepick. What's more, if i do have immutable, i'd be bummed if one of my deps forced me to use icepick.

icepick has 52k downloads vs. immutable's 2.9M. https://yarnpkg.com/en/packages?q=icepic&p=1 i think it's safe to say that if you want immutability, you're gonna use immutable. if you don't, then you probably just want the smallest vanilla thing you can get.

marcins commented 7 years ago

I was going to argue that Icepick is already pretty much vanilla (being some ~350 lines of source), but on further evaluation the only actual array method we need that isn't already immutable is shift. Working on a branch now with a vanilla implementation, should be ready soon!

mattkrick / redux-optimistic-ui

Remove dependency on Immutable? #21