Improve the performance of composing computations #128

Avaq · 2017-07-13T13:32:07Z

Instead of using an array, and forcing it to be "immutable" by using .concat([item]), I've defined a very simple immutable linked list and use that to build up the list of operations to perform during interpretation.

As an example, I took the stack safety example from Fluture's own README:

//test.js
const Future = require('.');

const add1 = x => x + 1;
let m = Future.of(1);

for(let i = 0; i < 100000; i++){
  m = m.map(add1);
}

m.fork(console.error, console.log);

Before

$ time node test.js 
100001

real    0m30.145s
user    0m23.878s
sys     0m8.083s

After

$ time node test.js 
100001

real    0m0.153s
user    0m0.148s
sys     0m0.019s

For this type of script, the performance improves significantly. In practice, this improvement will probably be encountered in scripts using this pattern I've seen here and there:

reduce((m, x) => m.chain(acc => reducer(acc, x)), Future.of(initial), xs)

The general performance as measured by the Doxbee testing suite has improved by about 11%.

codecov-io · 2017-07-13T13:32:12Z

Codecov Report

Merging #128 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #128   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          47     48    +1     
  Lines        1088   1097    +9     
=====================================
+ Hits         1088   1097    +9

Impacted Files	Coverage Δ
src/core.js	`100% <100%> (ø)`	⬆️
src/internal/interpreter.js	`100% <100%> (ø)`	⬆️
src/internal/const.js	`100% <100%> (ø)`	⬆️
src/internal/list.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af6a6db...0ebea29. Read the comment docs.

Avaq · 2017-07-13T13:33:08Z

src/internal/list.js

+  this.head = head;
+  this.tail = tail;
+  this.size = tail.size + 1;
+}


@safareli, you once suggested using a List over an Array. You may be interested in seeing it. :)

Avaq · 2017-07-13T13:37:39Z

src/internal/const.js

@@ -11,6 +11,6 @@ export const ordinal = ['first', 'second', 'third', 'fourth', 'fifth'];

 export const namespace = 'fluture';
 export const name = 'Future';
-export const version = 2;
+export const version = 3;


@davidchambers Note how I'm incrementing the version of the Fluture @@type. This is because in changing the internal structure of the Sequence instances, I've broken backwards compatibility. interpret expects _actions to be a List now, and not an Array.

Despite this technically being a "breaking change", I will not release it as a major version, but use a minor version instead. Detecting incompatible versions is really just there to improve error messages.

davidchambers · 2017-07-13T13:44:27Z

src/internal/list.js

+
+List.prototype.isEmpty = false;
+
+List.prototype.add = function List$add(x){


Why add rather than cons?

Because I have no idea about conventions. ;)
⚡

davidchambers · 2017-07-13T13:45:54Z

src/internal/list.js

+Empty.prototype.tail = null;
+Empty.prototype.size = 0;
+
+export default new Empty;


I find this export confusing. I would not expect require('./internal/list') to give me this value.

It's because I'm implementing the bare minimum, so to construct a list one does list.add(1).add(2).add(3). No need to export the internals, or a specialized constructor.

I suggest renaming the file empty.js, in that case.

You don't need Empty constructor, can just have special object created from List.prototype with desired props.

davidchambers · 2017-07-13T13:46:42Z

src/core.js

+  let str = this._spawn.toString(), tail = this._actions;
+
+  while(!tail.isEmpty){
+    str = `${str}.${tail.head.toString()}`;


Should we use Z.toString here?

We can safely avoid unnecessary dispatching, because we know that every item in this._actions has a custom toString function defined which we want to use. If not, that would be a bug in Fluture.

Avaq · 2017-07-17T08:34:57Z

src/internal/list.js

+empty.isEmpty = true;
+empty.size = 0;
+
+export default empty;


@davidchambers I'm still quite fond of exporting empty from list.js. Essentially we're exporting [] and : (cons), and those are all the necessary building blocks for creating lists. Not only do I feel like this is elegant in a way, it's also quite convenient because I don't have to do anything before using it the way I want to in core.js.

Avaq · 2017-07-17T11:15:24Z

src/internal/list.js

@@ -0,0 +1,2 @@
+export const empty = ({isEmpty: true, size: 0, head: null, tail: null});
+export const cons = (head, tail) => ({isEmpty: false, size: tail.size + 1, head, tail});


Surprisingly, defining the list type like this has zero impact on performance. What do you think @davidchambers and @safareli? I think it addresses both of your points of feedback. :)

Beautiful! 🦄

The last thing to consider is s/empty/nil/ to avoid confusion with Z.empty.

The parens around the object literal assigned to empty are not required (although you may have included them for aesthetic reasons).

I did include them for aesthetic reasons. :)
I'll keep it as empty, as I consider it more descriptive within the context of a list. I renamed it emptyList during import to core.js, to preserve context.

* #128 The performance of transforming Futures has improved significantly. * #131 Future.parallel has been made stack-safe. * #131 Future.parallel now guarantees execution order. * #132 Improve version interoperability. Note that 6.3.x futures will not be compatible with 6.2.x Futures, so when upgrading, make sure to upgrade all producers of Future instances. To avoid having to do this with future releases, I recommend setting Fluture as a devDependency if you are maintaining a library which exposes Future instances to its users.

Avaq commented Jul 13, 2017

View reviewed changes

davidchambers reviewed Jul 13, 2017

View reviewed changes

Avaq force-pushed the avaq/list branch from de90d49 to d672bce Compare July 13, 2017 14:12

Avaq commented Jul 17, 2017

View reviewed changes

Improve the performance of composing computations

0ebea29

Avaq force-pushed the avaq/list branch from 2255dbc to 0ebea29 Compare July 17, 2017 12:57

Avaq merged commit ba73ba8 into master Jul 17, 2017

Avaq deleted the avaq/list branch July 17, 2017 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the performance of composing computations #128

Improve the performance of composing computations #128

Avaq commented Jul 13, 2017

codecov-io commented Jul 13, 2017 •

edited

Loading

Avaq Jul 13, 2017 •

edited

Loading

safareli Jul 13, 2017

Avaq Jul 13, 2017

davidchambers Jul 13, 2017

Avaq Jul 13, 2017

davidchambers Jul 13, 2017

Avaq Jul 13, 2017

davidchambers Jul 13, 2017

safareli Jul 13, 2017

davidchambers Jul 13, 2017

Avaq Jul 13, 2017

Avaq Jul 17, 2017 •

edited

Loading

Avaq Jul 17, 2017

davidchambers Jul 17, 2017

Avaq Jul 17, 2017

safareli Jul 17, 2017


		List.prototype.isEmpty = false;

		List.prototype.add = function List$add(x){

		@@ -0,0 +1,2 @@
		export const empty = ({isEmpty: true, size: 0, head: null, tail: null});
		export const cons = (head, tail) => ({isEmpty: false, size: tail.size + 1, head, tail});

Improve the performance of composing computations #128

Improve the performance of composing computations #128

Conversation

Avaq commented Jul 13, 2017

codecov-io commented Jul 13, 2017 • edited Loading

Codecov Report

Avaq Jul 13, 2017 • 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

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

Avaq Jul 17, 2017 • 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

Choose a reason for hiding this comment

codecov-io commented Jul 13, 2017 •

edited

Loading

Avaq Jul 13, 2017 •

edited

Loading

Avaq Jul 17, 2017 •

edited

Loading