Refactor to classes #34

Avaq · 2016-10-17T11:45:43Z

This refactors the code from using closure capturing to using class properties. This way of working was inspired by the FunTask source and has several benefits:

Performance

chainRec 50% faster
cache 100% faster
race - 300% faster
parallel - 30% faster
All other functions - 10-20% faster

Introspectivity

Every type of Future can now have its own toString function associated with it. Casting a Future to String now shows something much closer to what the user originally typed.
This new introspective nature also allows for the creation of a more informative sanctuary-def type - something for another pull-request.

Other fixes

Besides refactoring, I also corrected some minor mis-behaviors:

The cancellation function of cached Futures no longer unsubscribes all listeners. Instead only the listeners for the specific call to fork which created the called cancel function will be unsubscribed. If there are other listeners left, the Future will keep running.
Fix some broken benchmarks.
When Future.or, Future.race, or Future.parallel settles, all remaining running Futures will be cancelled.
Now when a hooked Future is cancelled, its disposal Future is still executed, and immediately cancelled as well. This creates a much better chance for resources to be disposed, and allows a developer to forcefully get rid of resources in a much more logical place: The disposal cancellation function instead of the consumption cancellation function.
Future.recur was removed in favor of Future.chainRec

These changes are backwards-incompatible, and will be released under a new major version.

* Improve performance by 90% (almost twice as fast) * Fix cancellation semantics (per-fork cancellation) * Improve toString and inspect to show the state (similar to Promises)

* Improves performance by about 10% * Improves toString

* Improve performance by about 10% * Improve toString

* Improve performance by about 300% overall * Improve performance by about 3700% in the case where left resolves synchronously * Improve toString

* Improve performance by 10-40% * Improve toString * When left resolves before right, right will be automatically cancelled

But keep inspect the same

Now when a hooked Future is cancelled, its disposal Future is still executed, and immediately cancelled as well. This creates a much better chance for resources to be disposed, and allows a developer to forcefully get rid of resources in a much more logical place: The disposal cancellation function instead of the consumpsion cancellation function. Also improve toString and probably performance.

Impoves toString

An average increase in speed of 30% with cases ranging between 10% and 100%

codecov-io · 2016-10-17T11:58:55Z

Current coverage is 100% (diff: 100%)

No coverage report found for master at 289612b.

Powered by Codecov. Last update 289612b...ebc46b0

Avaq · 2016-10-17T15:46:00Z

fluture.js

+      error$invalidArgument(`Future.${method}`, 0, `have a "${method}" method`, m);
+    };
+  }
+


These will eventually disappear as I will depend on sanctuary-type-classes for FantasyLand dispatching, rewrite all other dispatchers to just construct their associated classes directly.

davidchambers · 2016-10-17T16:05:46Z

README.md

+
+  //We try to dispose gracefully.
+  conn.flushGracefully(err => {
+    if(err) return rej(err);


Are you writing return rej(err); to save yourself from typing { rej(err); return; }? If so, I urge you to reconsider as it makes the lambda's return value appear to be significant. I'd find this clearer:

if (err == null) { conn.close(); res(); } else { rej(err); }

davidchambers · 2016-10-17T16:23:07Z

fluture.js

+        }else{
+          isSync = false;
+          return;
+        }


Is this equivalent to the if/else above?

if (isSync !== true) { isSync = false; return; }

davidchambers · 2016-10-17T16:24:30Z

fluture.js

+        }
+      }
+      while(!state.done){
+        isSync = null;


Could we have isSync :: Boolean rather than isSync :: Nullable Boolean? The latter is confusing and could lead to subtle bugs.

isSync has three states; undetermined, true and false. Perhaps I could represent them with numbers.

Numbers would be an improvement. This would be even clearer:

// data Timing = Synchronous | Asynchronous | Undetermined // Synchronous :: Timing const Synchronous = 'Synchronous'; // Asynchronous :: Timing const Asynchronous = 'Asynchronous'; // Undetermined :: Timing const Undetermined = 'Undetermined';

That's gorgeous. :)
I'm going for numbers though. I just tested and strings seem to cost performance. Same reason I mentioned below. V8 likes TinyInt.

davidchambers · 2016-10-17T16:29:03Z

test/fluture.test.js

@@ -216,6 +249,13 @@ describe('Constructors', () => {
      expect(f).to.throw(error);
    });

+    it('has custom toString and inspect', () => {
+      const m = Future.encase((a) => (a), 1);


Why (a) => (a) rather than a => a?

Copy-pasting

jordalgo · 2016-10-17T21:38:52Z

fluture.js

+  /////////////////
+
+  function SafeFuture(computation){
+    this._computation = computation;


raw -> computation 👍

jordalgo · 2016-10-17T21:46:15Z

fluture.js

-      return function Future$ap$cancel(){ c1(); c2() };
-    });
+  CachedFuture.prototype._drainQueue = function CachedFuture$drainQueue(){
+    const q = this._queue, l = q.length, s = this._state, v = this._value;


This function is probably optimized for length, but this is another case when I think a minifier would be great so this function can be a bit easier to understand.

No need to name these such small names. I actually just did this to move property-access to outside of the loop.

jordalgo · 2016-10-17T21:50:50Z

fluture.js

-  function Future$bimap(f, g){
-    check$bimap(this, f, g);
+  CachedFuture.STATE = {
+    0: 'cold',


Why not flip these and have the keys be the names so the code that utilizes the state can be easier to understand?

Because this way the code can optimize variables for TinyInt. It's an untested assumption that this matters in performance though. I'll test and see. :)

Actually; I'm also asserting state > 1 here and there. Assigning numbers to the state encodes order, which I can then use to determine how far through its state a CachedFuture is in one simple operation.

jordalgo · 2016-10-17T22:09:57Z

fluture.js

-    return Future.encase(f, undefined);
-  };
+  FutureChain.prototype.toString = function FutureChain$toString(){
+    return `${this._parent.toString()}.chain(${showf(this._chainer)})`;


My first reaction to this convention was it's nice to see the parent string stringified but now I'm wondering about long Future chains and what usefulness that monster string would provide.

Yeah, I'm also curious. I will tag this with 4.0.0-beta1 and test it in my own code to find out. Knowing that recursion would be done by a single ChainRec, I'm assuming that most chains won't be longer than about 20, but I'll have to see how manageable it is

* Use switch-statements instead of if-statements for argument length * Do recursion via the name of the function rather than its location * Move the type-check to after currying, so it doesn't happen multiple times

Also improve performance by about 5%

Avaq · 2016-10-18T12:44:21Z

Think this is ready to merge @davidchambers @jordalgo ? :)

jordalgo · 2016-10-18T13:04:58Z

bench/curry.js

+}
+
+function manual3if(a, b, c, d){
+  if(arguments.length === 1) return util.unaryPartial(manual3if, a);


This obviously can wait for another PR/commit, but make this switch just to be consistent?

No. The idea here is to test the performance of if vs switch. That's why the function is called manual3if ^^

haha. Yup that makes sense 😛

jordalgo · 2016-10-18T13:44:19Z

fluture.js

+    try{ r = this._fn() }catch(e){ rej(e); return noop }
+    res(r);
+    return noop;
+  }


Can this be written as

try { res(this._fn()); } catch(e) { rej(e); } finally { return noop; }

?

We can't have the call to res reside inside the try block, as it will cause any subsequent actions triggered by calling res to also have their thrown Errors caught, causing the Future to enter the rejection branch after already being resolved... scary stuff.

Ah right! Maybe I'll add a unit test to try for this also. 👍

jordalgo · 2016-10-18T14:21:59Z

@Avaq This is looking good. Nice work!

davidchambers · 2016-10-18T21:02:18Z

I ❤️ the named states. The changes look great to me!

Avaq · 2016-10-27T09:41:35Z

@jordalgo I've tagged a release candidate. You can install it with npm i -S avaq/fluture#4.0.0-rc.1. I find that in my existing projects, the toString seems very reasonable, with long chains of Futures typically being around 100 lines when cast to String. Since these strings really only appear in error messages, and are very helpful to determine which Future is saying the error, I think it's totally worth it. That said, I'm curious what you'll find.

jordalgo · 2016-10-28T14:27:41Z

@Avaq Pulled it down and it looks good. The toString method is actually pretty nice when I tried invoking it directly on a long chain 👍 However (and just from a few minutes of tinkering) I wasn't able to get a long toString error message when passing some bad args to methods in the chain. Do you have a sample of when this gets used in the case of an error?

Avaq · 2016-10-29T11:59:55Z

const m = Future.of() //.long().chain()
Future(m)

jordalgo · 2016-10-31T15:36:09Z

Ok. Looks good, though still not sure how often it will get used :) This version looks good 👍

Avaq · 2016-10-31T16:49:44Z

It's been released; https://github.com/Avaq/Fluture/releases/tag/4.0.0

Avaq added 25 commits October 9, 2016 22:30

WIP: Convert several methods from higher-order style to class-style

5877bd3

Fix test-opt script

6eb1fe5

Refactor Future.cache

5727d43

* Improve performance by 90% (almost twice as fast) * Fix cancellation semantics (per-fork cancellation) * Improve toString and inspect to show the state (similar to Promises)

Remove Future#recur in favor of Future.chainRec

36bd594

Fix bench/curry

ea78cd8

Refactor chainRej

56144ca

* Improves performance by about 10% * Improves toString

Correct a typo

3e893c8

Refactor mapRej

0a43aa3

* Improve performance by about 10% * Improve toString

Refactor Future.bimap

f144ccc

* Improve performance by about 10% * Improve toString

Fix inspect methods

471ece7

Refactor Future.swap

b7294fa

Add race benchmark

f4bfa3d

Refactor Future.race

aa6a3a7

* Improve performance by about 300% overall * Improve performance by about 3700% in the case where left resolves synchronously * Improve toString

Add Future.or benchmark

e34f1a6

Refactor Future.or

7041f53

* Improve performance by 10-40% * Improve toString * When left resolves before right, right will be automatically cancelled

Refactor Future.fold

becca2d

Use normal toString semantics for CachedFuture

1e5482e

But keep inspect the same

Refactor Future.finally

f86f468

Impoves toString

Move stuff around

05f54e1

Refactor static utilities to introspective classes

b17a52a

Add Future.parallel benchmark

2cd49ef

Performance optimize Future.parallel

14157c6

An average increase in speed of 30% with cases ranging between 10% and 100%

Fix a bug in CachedFuture

72bc0dc

Restore code coverage

b223ff8

This was referenced Oct 17, 2016

Update: make variable names more descriptive #30

Closed

Consider TypeScript? #32

Closed

Avaq commented Oct 17, 2016

View reviewed changes

davidchambers reviewed Oct 17, 2016

View reviewed changes

jordalgo reviewed Oct 17, 2016

View reviewed changes

fluture.js

/////////////////

function SafeFuture(computation){

this._computation = computation;

Copy link

jordalgo Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raw -> computation 👍

jordalgo reviewed Oct 17, 2016

View reviewed changes

Avaq added 9 commits October 18, 2016 11:02

Improve curry benchmark, check perf diff between 3 ifs and switch

2ca9baf

Improve the Future#hook readme example

51f8930

Remove excess brackets from encase unit tests

ee9e247

Remove excess brackets from isFuture check

904c1be

Improve code for Future.encaseN functions

405bf3c

* Use switch-statements instead of if-statements for argument length * Do recursion via the name of the function rather than its location * Move the type-check to after currying, so it doesn't happen multiple times

Use named states in ChainRec

27afcc7

Use named states in CachedFuture

a4edcbf

Also improve performance by about 5%

Optimization: ok += 1 -> ok = ok + 1

fabb2fc

Replace "if a then a else b" ternary operators with "a or b"

ebc46b0

jordalgo reviewed Oct 18, 2016

View reviewed changes

Avaq merged commit 89f2ed7 into master Oct 18, 2016

Avaq deleted the av-classes branch October 19, 2016 13:37

Avaq mentioned this pull request Oct 20, 2016

Replace Future#promise() with Promises/A+ compatible Future#then() #35

Closed

Avaq added this to the 4.0.0 milestone Oct 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to classes #34

Refactor to classes #34

Avaq commented Oct 17, 2016 •

edited

Loading

codecov-io commented Oct 17, 2016 •

edited

Loading

Avaq Oct 17, 2016

davidchambers Oct 17, 2016

davidchambers Oct 17, 2016

davidchambers Oct 17, 2016

Avaq Oct 18, 2016

davidchambers Oct 18, 2016

Avaq Oct 18, 2016

davidchambers Oct 17, 2016

Avaq Oct 18, 2016

jordalgo Oct 17, 2016

jordalgo Oct 17, 2016 •

edited

Loading

Avaq Oct 18, 2016

jordalgo Oct 17, 2016

Avaq Oct 18, 2016 •

edited

Loading

Avaq Oct 18, 2016

jordalgo Oct 17, 2016

Avaq Oct 18, 2016

Avaq commented Oct 18, 2016

jordalgo Oct 18, 2016

Avaq Oct 18, 2016

jordalgo Oct 18, 2016

jordalgo Oct 18, 2016

Avaq Oct 18, 2016

jordalgo Oct 18, 2016

jordalgo commented Oct 18, 2016

davidchambers commented Oct 18, 2016

Avaq commented Oct 27, 2016 •

edited

Loading

jordalgo commented Oct 28, 2016

Avaq commented Oct 29, 2016

jordalgo commented Oct 31, 2016

Avaq commented Oct 31, 2016

Refactor to classes #34

Refactor to classes #34

Conversation

Avaq commented Oct 17, 2016 • edited Loading

codecov-io commented Oct 17, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordalgo Oct 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq Oct 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avaq commented Oct 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordalgo commented Oct 18, 2016

davidchambers commented Oct 18, 2016

Avaq commented Oct 27, 2016 • edited Loading

jordalgo commented Oct 28, 2016

Avaq commented Oct 29, 2016

jordalgo commented Oct 31, 2016

Avaq commented Oct 31, 2016

Avaq commented Oct 17, 2016 •

edited

Loading

codecov-io commented Oct 17, 2016 •

edited

Loading

jordalgo Oct 17, 2016 •

edited

Loading

Avaq Oct 18, 2016 •

edited

Loading

Avaq commented Oct 27, 2016 •

edited

Loading