1602 / jugglingdb

Multi-database ORM for nodejs: redis, mongodb, mysql, sqlite3, postgresql, arango, in-memory...
http://1602.github.io/jugglingdb/
2.05k stars 241 forks source link

Finished promised version of Jugglindb #380

Closed pocesar closed 10 years ago

pocesar commented 10 years ago

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 commented 10 years ago

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

yanickrochon commented 10 years ago

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 commented 10 years ago

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/1602/jugglingdb/issues/380#issuecomment-35621564 .

pocesar commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

@faceleg listen for initialization event for each model.

pocesar commented 10 years ago

@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 commented 10 years ago

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

pocesar commented 10 years ago

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

faceleg commented 10 years ago

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

anatoliychakkaev commented 10 years ago

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 https://github.com/1602/jugglingdb/issues/380#issuecomment-47181780.

Thanks, Anatoliy Chakkaev

faceleg commented 10 years ago

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

anatoliychakkaev commented 10 years ago

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: https://github.com/1602/jugglingdb/issues/380#issuecomment-47281718

pocesar commented 10 years ago

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

anatoliychakkaev commented 10 years ago

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 https://github.com/1602/jugglingdb/issues/380#issuecomment-47342413.

Thanks, Anatoliy Chakkaev

pocesar commented 10 years ago

@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 commented 10 years ago

@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 commented 10 years ago

@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 commented 10 years ago

@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.

faceleg commented 10 years ago

@pocesar @1602 looking forward to the benchmark!

pocesar commented 10 years ago

@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 commented 10 years ago

@pocesar thanks for the tip, looks good!