omniscientjs / immstruct

Immutable data structures with history for top-to-bottom properties in component based libraries like React. Based on Immutable.js
374 stars 21 forks source link

listListenerMatching uses a tree for improved performance #55

Closed andrewluetgers closed 9 years ago

andrewluetgers commented 9 years ago

This PR actually does two things I know this is not scoped perfectly and could be two PRs but I'd like to get some input on this before I do any more work.

First is, what I think is a significant performance improvement and second is what I think is a meaningful interface simplification.

listListenerMatching optimization

This optimization was the result of experimenting with a custom omniscient mixin that allows you to do reference observer based updating of components directly as opposed to the usual top down approach. https://github.com/omniscientjs/omniscient/issues/93#issuecomment-84812169 I was able to achieve 80 fps using what would be the normal approach with immstruct, with the omniscient mixin I was able to get 100 fps and with this change to the underlying datastructure used by listListenerMatching I was able to achieve 170 fps. As a reference the fastest this app can operate on my machine is at 200 fps using vanilla js. See the above link for running code samples.

improved path expression

deep paths can now be provided as strings like 'foo.bar.baz' as well as ['foo', 'bar', 'baz']

andrewluetgers commented 9 years ago

sorry about the messy build file, how should I run the build to strip comments?

mikaelbr commented 9 years ago

Hi!

That sound like a really good performance gain, and something we are definitely interested in getting fixed.

Some comments:

The performance gain is pretty sweet by using trees instead of a list, but it's hard distinguishing the code for what does this, so If you clean up your commit (ref. comments), it will be much easier to merge. Thanks for your help!

andrewluetgers commented 9 years ago

Glad to clean up and remove the paths as strings stuff, I was actually just messing around and not thinking I would create a PR untill I saw the performance improvement. However I just pushed a PR to immutable.js for paths as strings. As an option not the sole approach I think a simplistic handling is reasonable, but will leave that for a later PR when and if it gets accepted in immutable.js. If it is not accepted there it makes little sense to use it here.

andrewluetgers commented 9 years ago

Ahh sorry about that, was not intending on committing the undefined stuff, its an odd thing to see void 0 crop up, I get why it gets used, just have never seen undefined actually ever get clobbered.