search

optimizely / nuclear-js

Reactive Flux built with ImmutableJS data structures. Framework agnostic.
https://optimizely.github.io/nuclear-js/
MIT License
2.23k stars 141 forks source link

Refactor evaluator to cache getters by reference, not hash code #159

Closed jordangarcia closed 9 years ago

jordangarcia commented 9 years ago

This fixes #140.

My concern here is about increased memory usage, will run a few other benchmarks to make sure this is okay.

@bhamodi @andrewdavey

bhamodi commented 9 years ago

2 minor comments above to address, but other than that, it LGTM. I'd also be interested in seeing the change in performance. Could you post some stats?

andrewdavey commented 9 years ago

Can this be merged and a new build released? Thanks.

jordangarcia commented 9 years ago

Unfortunately still needs a bit of work. The more aggressive caching mechanism lead to too many things being held on to by reference which caused memeory leaks.

I am working on refactoring this in a different branch now. Sorry for the inconvenience.

bhamodi commented 9 years ago

Any updates on this @jordangarcia?

jakecraige commented 9 years ago

We also ran into this today and it took a good hour or so trying to debug. Would love to see this change come through.

Our test case was that null and 0 in a map returns the same hash code so the getter was never invalidated:

Immutable.Map({something: null}).hashCode() === Immutable.Map({something: 0}).hashCode() // true
bhamodi commented 9 years ago

This has been done in v1.2.0.