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

Finished promised version of Jugglindb #380

Closed
pocesar opened this issue Feb 20, 2014 · 22 comments
Closed

Finished promised version of Jugglindb #380

pocesar opened this issue Feb 20, 2014 · 22 comments

Comments

@pocesar
Copy link
Contributor

pocesar commented Feb 20, 2014

Every test is passing https://travis-ci.org/pocesar/promised-jugglingdb/builds/19241763

This version is a dual API version, it means it can use both "old" callback or promise. deprecated code was removed, I've cleaned up the code (for my own liking).

I'll will finish writing the dualapi.test.js to conform with everything and ensure that both ways to use the module works (either by callback or promise)

code is in here https://github.com/pocesar/promised-jugglingdb/tree/promises

I will also ensure that the library gets 100% test coverage https://coveralls.io/r/pocesar/promised-jugglingdb?branch=master

@1602
Copy link
Owner

1602 commented Feb 20, 2014

Cool news. Thanks! Any actions required from my side?

@yanickrochon
Copy link

Well... I think this should have been put into a branch rather than a separate project, so it can be merge more easily instead of maintaining it manually. Don't you think?

@anatoliychakkaev
Copy link
Collaborator

I think promises should stay as jugglingdb add-on without any changes in
core, because it is not possible to add promises without additional
overhead on creating each object. So, it's up to user to choose whether to
use promises or not and it should not be included in core.

On 20 February 2014 13:35, Yanick Rochon notifications@github.com wrote:

Well... I think this should have been put into a branch rather than a
separate project, so it can be merge more easily instead of maintaining it
manually. Don't you think?

Reply to this email directly or view it on GitHubhttps://github.com//issues/380#issuecomment-35621564
.

@pocesar
Copy link
Contributor Author

pocesar commented Feb 21, 2014

I intend to maintain it because I will be using it on my framework https://github.com/socket-express/core
instead of monkey patching, decided to implement it in core, plus I can ensure code quality (by my own standards of course). I think would be nice to have a "mention" in Jugglingdb (this repo) readme about the dual API fork.

@faceleg
Copy link

faceleg commented Jun 25, 2014

@anatoliychakkaev "I think promises should stay as jugglingdb add-on without any changes in
core"

Is there a way that an addon could be created that grafted promised onto core? I'd like to use a promisified version of juggling, but would prefer to be able to continue tracking core...

@1602
Copy link
Owner

1602 commented Jun 25, 2014

@faceleg listen for initialization event for each model.

@pocesar
Copy link
Contributor Author

pocesar commented Jun 25, 2014

@1602 @faceleg that's where the real overhead happens, because you need to execute promisifyAll on the initialized model every time, instead of having a set of promisified prototype. I measured it before doing the promise fork, it's almost 8 times slower promisifying the instance methods

@faceleg
Copy link

faceleg commented Jun 25, 2014

@pocesar is it 8 times slower every time one instantiates a model, or just on bootstrap?

@pocesar
Copy link
Contributor Author

pocesar commented Jun 26, 2014

everytime, since there's no bootstrap. everytime you do User.create() for example

@faceleg
Copy link

faceleg commented Jun 26, 2014

@1602 is there no way to allow this to happen when a model is declared?

@anatoliychakkaev
Copy link
Collaborator

It can not be declared on prototype, because you need particular instance,
not prototype, otherwise your promises will affect all instances. And this
is the same as hook on initialize, that means - slowdown overall jugglingdb
speed. No other way around. Let's close this thread.

On 26 June 2014 03:30, Michael Robinson notifications@github.com wrote:

@1602 https://github.com/1602 is there no way to allow this to happen
when a model is declared?


Reply to this email directly or view it on GitHub
#380 (comment).

Thanks,
Anatoliy Chakkaev

@faceleg
Copy link

faceleg commented Jun 26, 2014

@pocesar does your fork have this slowdown? If not, @anatoliychakkaev why can't it be merged with core?

@anatoliychakkaev
Copy link
Collaborator

It can't be merged because it's slow.

On Thu, Jun 26, 2014 at 10:20 PM, Michael Robinson
notifications@github.com wrote:

@pocesar does your fork have this slowdown? If not, @anatoliychakkaev why can't it be merged with core?

Reply to this email directly or view it on GitHub:
#380 (comment)

@pocesar
Copy link
Contributor Author

pocesar commented Jun 27, 2014

@faceleg there's 30-40ms difference between calls, maybe @1602 haven't used promises in a while, bluebird is pretty performant

@anatoliychakkaev
Copy link
Collaborator

30ms is huge amount of time, in our API we serve full request for 50ms
(including several database calls and additional logic). Any slowdown in
exchange to syntax sugar is not acceptable.

On 27 June 2014 14:22, Paulo Cesar notifications@github.com wrote:

@faceleg https://github.com/faceleg there's 30-40ms difference between
calls, maybe @1602 https://github.com/1602 haven't used promises in a
while, bluebird is pretty performant


Reply to this email directly or view it on GitHub
#380 (comment).

Thanks,
Anatoliy Chakkaev

@pocesar
Copy link
Contributor Author

pocesar commented Jun 27, 2014

@anatoliychakkaev notice that I'm benching it on a Core 2 Quad processor and not a server. difference should negligible in a server processor

@randunel
Copy link
Collaborator

@pocesar Try testing in a VM if you are extremely bored, users are more likely to run node.js in virtual machines, rather than barebone servers. Anyway, I stand by the argument that any slowdown (even 1ms) for syntactic sugar is a bad idea.

@pocesar
Copy link
Contributor Author

pocesar commented Jun 27, 2014

@randunel the "problem" is that promises aren't sugar, it's functional programming, they enable you to do proper code flow. generators are coming, and they are not sugar as well and they try to tame the async nature of Javascript. promises aren't like using class on ES6 or fat arrow =>, that's real sugar. coffeescript is syntax sugar.
need to have better understanding about the technology and reasoning about change in programming paradigm (it's a real problem in node called callback hell). it's like ditching streams because they are slower than procedural javascript, makes no sense.

But to assure you, I'm going to create a real benchmark on my fork, and execute it in a docker container. It's because I'm bored, it's because I believe in a better Javascript library, else I would have given up trying to monkey patch Jugglingdb long time ago, and rolled with my own code, or even used Juggler from StrongLoop. If you have no idea, this is a good read https://blog.jcoglan.com/2013/03/30/callbacks-are-imperative-promises-are-functional-nodes-biggest-missed-opportunity/

@1602
Copy link
Owner

1602 commented Jun 28, 2014

@pocesar callback hell is not problem of node or javascript. it's a problem of programmer not using programming language properly. You can always avoid callback hell if you organize your code. One of the ways to organize code is to use promises. Personally I don't like promises, because they tend to hide async nature of code, and surprises unprepared reader, which is bad; code is not obvious anymore and this is even worse than callback hell. Promise is a perfect example of solving existing design problem by creating another design problem. I vote for 'natural' way of using programming language: sync sections should look sync, and async should look async.

But the main concern here - inefficiency. We have to perform additional operations on object every instance you've created, this is limitation of javascript implementation which has no 'method missing' mechanism on objects. So, you have to add some overhead. It may work for some programmers, and I don't mind if: 1) my code will still work fast 2) this programmer is not part of my team, so I don't have to read his code full of promises. In our case (1) is violated, because my code will be slower than it was before.

But jugglingdb API allows both of us to be happy. It has object initialization hook, so that you can do anything you want with object after creation, just intercept initialize hook in your code.

@1602 1602 closed this as completed Jun 28, 2014
@faceleg
Copy link

faceleg commented Jun 29, 2014

@pocesar @1602 looking forward to the benchmark!

@pocesar
Copy link
Contributor Author

pocesar commented Jun 29, 2014

@faceleg After comparing both https://github.com/strongloop/loopback-datasource-juggler and the current state from JugglingDB, I guess it's much better to go with Juggler, since it's well maintained, follow good programming practices, don't overuse closures, don't hide prototypes, don't create nested definitions, it's almost free from spaghetti code. It also have better visibility since it has an important company behind it (StrongLoop). I'm pretty sure to move on after @1602 statement. But about the benchmark, I'll do with with Juggler, and don't bother with JugglingDB, since it's contradictory, see #356
He says 1ms is important, yet, he's using Function.prototype.bind everywhere, to 2300% times slower code.

@faceleg
Copy link

faceleg commented Jul 1, 2014

@pocesar thanks for the tip, looks good!

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

No branches or pull requests

6 participants