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

New Algorithm #37

Merged
merged 199 commits into from
Mar 7, 2017
Merged

New Algorithm #37

merged 199 commits into from
Mar 7, 2017

Conversation

ds300
Copy link
Owner

@ds300 ds300 commented Apr 24, 2016

Up until now, DerivableJS used a mark-and-sweep style algorithm to both propagate change and free unused memory.

This had a couple of minor performance implications, and one obscure-but-serious memory leak vulnerability (see #36).

This reimplementation of DerivableJS' core data structures removes the mark-and-sweep stuff, replacing it with a significantly simpler system inspired by Clojure's epochal STM.

The practical implications are:

  • atoms never ever store references to derivations. Only to reactors. This is ideal, and solves the memory management issue once and for all.
  • the time complexity of updating atoms is now linear in the number of dependent reactors rather than in the number of dependent (derivations + reactors), and should be much more performant due to cache locality goodness.
  • the cost of dereferencing derivables should be either similar or faster than before, but I'll have to do benchmarking to be sure of this.
  • smaller file size (.min.gz has gone from 4,330 bytes down to 3,718 bytes. Makes all the difference.)

The progress of this reimplementation is as follows:

  • typescript prototype of epochal system to verify viability
  • retrofit DJS with epochal system
  • get all the unit tests passing
  • trial epochal system in real-world projects already using DJS
  • modernize code base (npm build scripts, es6 modules + rollup, CI)
  • do white box testing. get 100% test coverage.
  • check performance and rework if necessary
  • modify read-me to talk about push-pull as opposed to mark-and-sweep, like Agera
  • release

@willmruzek
Copy link

Does the flux challenge you've written use CQRS/redux?

@ds300
Copy link
Owner Author

ds300 commented Aug 15, 2016

Neither, although the approach is very similar indeed. The main difference from Redux is that the actions are implicit so they're not serializable. The reducer pattern is more-or-less intact. Porting the code to redux should be super easy.

@willmruzek
Copy link

I'll just give that a look and maybe convert it to Redux as an exercise.

@oskbor
Copy link
Contributor

oskbor commented Sep 1, 2016

So close now! When will this land?

@ds300
Copy link
Owner Author

ds300 commented Sep 1, 2016

Not sure. I was planning to make nice new docs and a respectable-looking landing page before pushing out a 1.0.0 release. This was partly done, then summer happened and I got put on a very fulfilling project at work so my motivation to code on evenings and weekends has been waning.

However I've lately been learning the value of MVPs and thinking that it would be better to put out something more lean (docs-wise) to begin with at least. But still need to wait until I have some spare time at a weekend, which is looking like early October right now.

@zerkalica
Copy link

zerkalica commented Sep 1, 2016

Can you shortly explain about breaking changes in feature release? What should be avoided in the current version?

@ds300
Copy link
Owner Author

ds300 commented Sep 1, 2016

The main breaking change is that the Reactor class and its associated API (Derivation#reactor) have gone away.

The top-level withEquality function was removed too.

Behaviour-wise, laziness is now only provided for 'active' derivations (which are upstream of an 'active' reactor). I might still change this back to the old behaviour (always lazy) before 1.0.0 is released if I can find a way to do it without impacting performance for active derivations.

I think everything else is the same.

@zerkalica
Copy link

I try to test 1.0.0-beta11, but catch strange bug: derive does not cache result, each get recalculate value again. It's some optimizations or bug?

        const a = atom({p: 1})

        const b = a.derive((p) => {
            console.log('test')
        })

        b.get() // "test"
        b.get() // error: "test" again

@ds300
Copy link
Owner Author

ds300 commented Oct 4, 2016

This is expected behaviour. Derivations are now only cached when the
derivation is being actively used by a downstream reactor. This enabled
some good performance enhancements, but I still would like to bring caching
back for all derivations somehow before it leaves beta.

On Tue, 4 Oct 2016, 01:22 Stefan, notifications@github.com wrote:

I try to test 1.0.0-beta11, but catch strange bug: derive does not cache
result, each get recalculate value again. It's some optimizations or bug?

    const a = atom({p: 1})

    const b = a.derive((p) => {
        console.log('test')
    })

    b.get() // "test"
    b.get() // error: "test" again


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#37 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABL1qWVHBIlca888kMTZWW2kfZTi9xFDks5qwY5NgaJpZM4IOY1p
.

@andreypopp
Copy link
Contributor

but I still would like to bring caching
back for all derivations somehow before it leaves beta.

Any idea how to achieve that? I'd be interested to work on a pull request.

@ds300
Copy link
Owner Author

ds300 commented Oct 7, 2016

@andreypopp The epochal system I started this PR with would work if kind-of superimposed over the current setup. That could end up being quite complex to implement and test, but it should add only a small amount of constant overhead. A PR would be super cool if you're feeling intrepid :) Let me know if you want any help figuring stuff out!

@oskbor
Copy link
Contributor

oskbor commented Dec 14, 2016

How is the rewrite coming along? ☺️

@ds300
Copy link
Owner Author

ds300 commented Dec 14, 2016 via email

@oskbor
Copy link
Contributor

oskbor commented Dec 15, 2016 via email

@ds300
Copy link
Owner Author

ds300 commented Mar 7, 2017

This PR is ridiculous. I'm just gonna merge it into master and start doing feature and release branches.

@ds300 ds300 merged commit cb5e493 into master Mar 7, 2017
@TrySound TrySound deleted the new-algo branch November 10, 2017 11:00
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.

7 participants