Useful stuff #20

bergus · 2016-07-11T19:06:36Z

…that occurred to me during working on cancellation (#21) and handles (#9).
Feel free not to like 130aed7 ("better module structure") and I'll drop it from this PR. The other commits shouldn't be so controversial I hope.

coveralls · 2016-07-11T19:10:52Z

Coverage remained the same at 100.0% when pulling 130aed7 on bergus:master into 008b3b9 on briancavalier:master.

briancavalier · 2016-07-12T01:40:07Z

perf/doxbee-sequential/promises-creed-algebraic.js

@@ -52,7 +52,7 @@ module.exports = function upload(stream, idOrPath, tag, done) {
    }).chain(function() {
        return File.whereUpdate({id: fileId}, {version: version.id})
            .execWithin(tx);
-    }).map(function() {
+    }).then(function() {


Good catch.

briancavalier · 2016-07-12T01:44:25Z

Thanks @bergus.

Feel free not to like 130aed7 ("better module structure") and I'll drop it from this PR

I'm def not opposed to improving the module structure. I think it would be easier to review in smaller chunks, though. So, yeah, let's deal with 130aed7 separately. Thanks!

coveralls · 2016-07-13T02:52:52Z

Coverage remained the same at 100.0% when pulling e7bb832 on bergus:master into a84df67 on briancavalier:master.

coveralls · 2016-07-13T02:54:52Z

Coverage remained the same at 100.0% when pulling c2c9f26 on bergus:master into a84df67 on briancavalier:master.

bergus · 2016-07-13T02:56:45Z

Rebased and ready to merge (fast-forward?).
ES6 build for performance measurement is now just done via a separately invocable script that overwrites dist/main.js when run.

briancavalier · 2016-07-13T12:39:38Z

package.json

@@ -27,12 +27,13 @@
  ],
  "scripts": {
    "build-dist": "mkdirp dist && rollup -c",
-    "build": "npm run build-dist && uglifyjs -c 'warnings=false' -m -o dist/creed.min.js -- dist/creed.js",
+	"build-es6": "mkdirp dist && rollup -f cjs -o dist/creed.js src/main.js",


indentation

briancavalier · 2016-07-13T13:14:05Z

Thanks for rebasing this into a smaller chunk. I made some comments on the source, but haven't made it through the tests yet. I should have time tonight to review those.

briancavalier · 2016-07-13T13:17:00Z

package.json

    "preversion": "npm run build",
    "check-coverage": "istanbul check-coverage --statements 100 --branches 100 --lines 100 --functions 100 coverage/coverage*.json",
    "lint": "jsinspect src && eslint src",
    "pretest": "npm run lint",
-    "test": "istanbul cover _mocha",
+    "test": "istanbul cover node_modules/mocha/bin/_mocha",


This shouldn't be necessary, since _mocha gets linked in node_modules/.bin/ (I've used istanbul cover _mocha in quite a few other projects without any problem). Did you have problems with it?

Yes, like many others: gotwarlost/istanbul/issues/90. The problem is that on Windows, node_modules/.bin/_mocha is a bash file and istanbul throws when trying to load it as a js module.

Drat, windows 🔫 😭 Oh well, good to know!

* fix quotes of argument to uglify * not sure whether specifying the path node_modules/mocha/bin/ is an antipattern (some comments by npm authors claim so) but istanbul does not seem to take the PATH into account

coveralls · 2016-07-23T15:50:19Z

Coverage remained the same at 100.0% when pulling 0d44e84 on bergus:master into 67f1ac1 on briancavalier:master.

coveralls · 2016-07-23T16:07:25Z

Coverage remained the same at 100.0% when pulling fd03346 on bergus:master into 67f1ac1 on briancavalier:master.

bergus · 2016-07-23T18:52:07Z

Ready to merge now, I think.

briancavalier · 2016-07-28T01:26:28Z

test/future-test.js

+				return assertSame(p, res)
+			})
+
+			/* it('should have never state for never', () => {


Hmmm, I might have missed these isNever-related tests. Why are they commented out?

They're commented out because they all fail :-) See #30
This is a good example for a difference between futures that are resolved before the method call and those that are resolved "asynchronously".

briancavalier · 2016-07-28T01:30:58Z

Thanks for being patient with my radio silence lately :) I left a couple more comments: one about silencer, and another with a question about some commented-out tests that I just noticed.

briancavalier · 2016-07-28T01:46:58Z

Hmmm, and oddly enough, I'm seeing more difference between the _runAction-based and _when-based coroutine continuations in this branch. It doesn't seem to show up in the doxbee test, but in the parallel test, using _when seems to end up being roughly 10% slower on average. That's definitely not the end of the world, but any thoughts on what might be causing it?

just use "npm run build-es6" instead of "npm run build" and the /dist/creed.js file will contain native ES6 except for the module syntax benchmarks will automatically use it instead of /src/main.js as entry point

closes briancavalier#31.

coveralls · 2016-07-29T05:16:36Z

Coverage remained the same at 100.0% when pulling 09b0566 on bergus:master into 0f16a8e on briancavalier:master.

bergus · 2016-07-29T06:58:14Z

Considering your comment, I did some benchmarks myself as well. And indeed, the parallel benchmark does show an increase in both runtime (780->620ms) and memory (116->119MB) for the promises-creed-generator test case, comparing origin against my master branch. (The promise-creed test case stays quite constant at 690ms/111MB). So I tried a lot of things:

_when vs _runAction in coroutines - no difference at all
Coroutine extending Action vs as a standalone class - no difference
handle inside try vs outside - no difference
Indexed extending Action vs standalone class - no difference
Indexed class vs the old object literal solution - bummer! Back at 780ms/116MB. However, at the same time the non-generator test case grew to 117MB?!

This kinda makes sense as doxbee does not use iterables while parallel uses the all function, so that's what makes the difference. I do however not understand why or how this happens, and rather not have to speculate.
I do however believe that an Action subclass is the cleanest solution and a compiler should eventually optimise for that.

briancavalier · 2016-08-01T13:17:44Z

Hmmm, strange. I was able to observe a 10% difference by just flipping back and forth between _when and _runAction in this branch using node 6.3.0 and 6.3.1. Again, I don't see it as a big deal, just seems weird that I don't observe the same difference when flipping back and forth in current creed 1.0.2.

IOW, on my laptop in node 6.3.x, when the only variable is _when vs. _runAction, this branch shows a perf difference which master doesn't. That probably means that there is some interaction between the various changes in this branch, plus maybe some differences in windows vs. OS X.

I agree Indexed is a cleaner solution. I'm cool with it.

briancavalier · 2016-08-01T13:18:22Z

src/inspect.js

@@ -47,6 +47,7 @@ export function silenceError (p) {
 	p._runAction(silencer)
 }

+// implements Action


👍 Thanks for adding this comment, too.

briancavalier · 2016-08-01T13:19:36Z

I'll give this another quick once-over tonight, and then hopefully merge it.

briancavalier · 2016-08-02T12:45:56Z

Have you thought about how we can implement bifunctor with Action?

bergus · 2016-08-02T13:14:32Z

I guess you'd have to do

class Bimap extends Action {
  constructor(f, r, promise) {
    super(promise)
    this.f = f
    this.r = r
    this.handle = null
  }
  fulfilled(p) {
    this.handle = this.handleFulfilled
    this.tryCall(this.f, p.value)
  }
  rejected(p) {
    this.handle = this.handleRejected
    this.tryCall(this.r, p.value)
  }
  …
}

The alternative would be the to pass this.handle as an argument to tryCall directly, and do so in every Action subclass (and in case of Bifunctor, pass either handleFulfill or handleReject).
I don't know which one would be the more reasonable (or better optimised) choice.

briancavalier · 2016-08-09T15:33:18Z

Thanks for the thoughts on bifunctor. I think we can merge this ahead of #34, and figure out a good way to implement bifunctor in the new Action-based world. We can probably refactor to parameterize a bit more.

briancavalier · 2016-08-09T15:34:00Z

Thanks for all the work on this, @bergus!

bergus · 2016-08-12T12:42:12Z

I just tried merging this and am curious why you did rebase/squash all these commits instead of doing a regular merge? This kinda fragments my history :-/

briancavalier reviewed Jul 12, 2016
View reviewed changes

bergus force-pushed the master branch from 130aed7 to e7bb832 Compare July 13, 2016 02:51

bergus force-pushed the master branch from e7bb832 to c2c9f26 Compare July 13, 2016 02:53

briancavalier reviewed Jul 13, 2016
View reviewed changes

make npm scripts work under Windows

d7a5e23

* fix quotes of argument to uglify * not sure whether specifying the path node_modules/mocha/bin/ is an antipattern (some comments by npm authors claim so) but istanbul does not seem to take the PATH into account

bergus force-pushed the master branch from c2c9f26 to 0d44e84 Compare July 23, 2016 15:49

bergus mentioned this pull request Jul 23, 2016

Implement bifunctor #34

Merged

2 tasks

briancavalier reviewed Jul 28, 2016
View reviewed changes

generalise actions

7dbaf6d

bergus added 3 commits July 29, 2016 07:14

fixes and additions to test suite

9fc6805

enable ES6 syntax for profiling

571bf3a

just use "npm run build-es6" instead of "npm run build" and the /dist/creed.js file will contain native ES6 except for the module syntax benchmarks will automatically use it instead of /src/main.js as entry point

always resume coroutines asynchronously

09b0566

closes briancavalier#31.

bergus force-pushed the master branch from fd03346 to 09b0566 Compare July 29, 2016 05:15

briancavalier reviewed Aug 1, 2016
View reviewed changes

briancavalier merged commit 42a81f0 into briancavalier:master Aug 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Useful stuff #20

Useful stuff #20

bergus commented Jul 11, 2016 •

edited

coveralls commented Jul 11, 2016 •

edited

briancavalier Jul 12, 2016

briancavalier commented Jul 12, 2016

coveralls commented Jul 13, 2016 •

edited

coveralls commented Jul 13, 2016 •

edited

bergus commented Jul 13, 2016

briancavalier Jul 13, 2016

bergus Jul 13, 2016

briancavalier commented Jul 13, 2016

briancavalier Jul 13, 2016

bergus Jul 13, 2016

briancavalier Jul 13, 2016 •

edited

coveralls commented Jul 23, 2016 •

edited

coveralls commented Jul 23, 2016 •

edited

bergus commented Jul 23, 2016

briancavalier Jul 28, 2016

bergus Jul 28, 2016

briancavalier commented Jul 28, 2016

briancavalier commented Jul 28, 2016

coveralls commented Jul 29, 2016 •

edited

bergus commented Jul 29, 2016

briancavalier commented Aug 1, 2016

briancavalier Aug 1, 2016

briancavalier commented Aug 1, 2016

briancavalier commented Aug 2, 2016

bergus commented Aug 2, 2016

briancavalier commented Aug 9, 2016

briancavalier commented Aug 9, 2016

bergus commented Aug 12, 2016

Useful stuff #20

Useful stuff #20

Conversation

bergus commented Jul 11, 2016 • edited

coveralls commented Jul 11, 2016 • edited

Choose a reason for hiding this comment

briancavalier commented Jul 12, 2016

coveralls commented Jul 13, 2016 • edited

coveralls commented Jul 13, 2016 • edited

bergus commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briancavalier commented Jul 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briancavalier Jul 13, 2016 • edited

Choose a reason for hiding this comment

coveralls commented Jul 23, 2016 • edited

coveralls commented Jul 23, 2016 • edited

bergus commented Jul 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briancavalier commented Jul 28, 2016

briancavalier commented Jul 28, 2016

coveralls commented Jul 29, 2016 • edited

bergus commented Jul 29, 2016

briancavalier commented Aug 1, 2016

Choose a reason for hiding this comment

briancavalier commented Aug 1, 2016

briancavalier commented Aug 2, 2016

bergus commented Aug 2, 2016

briancavalier commented Aug 9, 2016

briancavalier commented Aug 9, 2016

bergus commented Aug 12, 2016

bergus commented Jul 11, 2016 •

edited

coveralls commented Jul 11, 2016 •

edited

coveralls commented Jul 13, 2016 •

edited

coveralls commented Jul 13, 2016 •

edited

briancavalier Jul 13, 2016 •

edited

coveralls commented Jul 23, 2016 •

edited

coveralls commented Jul 23, 2016 •

edited

coveralls commented Jul 29, 2016 •

edited