Skip to content

Add Pool option initialSize which create connections when the pool is started. #499

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

Closed
wants to merge 7 commits into from
Closed

Conversation

ifsnow
Copy link
Contributor

@ifsnow ifsnow commented May 24, 2013

(Please excuse my poor English. It's my first pull request. :])

I have been working to improve the pool features.

  1. Add Pool option initialSize which create connections when the pool is started.
  2. .bind() change to closure function. (closure is 10x faster than bind)
  3. Clear the remaining queries from connection(queue) before release the connection.
  4. Fix some with refactoring.

ifsnow added 2 commits May 23, 2013 16:52
…ol is started.

2. .bind() change to closure function. (closure is 10x faster than bind)
3. Fix some with refactoring.
@felixge
Copy link
Collaborator

felixge commented May 24, 2013

Thanks for all the work! 💖

It looks good to me on first sight, but I won't be able to do a full review. Maybe @dresende @NateLillich can comment as well?

About the performance tweak for bind: How often is the relevant code executed? Unless it's >= 1 Mhz it's probably not worth to micro optimise stuff.

Last but not least: I've given you commit access to the project. Once you're happy with your patch, feel free to merge it in.

@dresende
Copy link
Collaborator

I'm out of time now but will certainly look at it. Feel free to push it, Pool really needs some love, there are too many open issues about it.

@tellnes
Copy link
Collaborator

tellnes commented May 24, 2013

I wonder when do you need to have a fixed amount of connections set up before use? Why is that better than just lazy create them when needed?

@@ -307,3 +307,8 @@ Protocol.prototype._debugPacket = function(incoming, packet) {
console.log(packet);
console.log('');
};

Protocol.prototype.reset = function() {
this._queue = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't this lead to bugs where callbacks is not called?
What about emiting an error if there is any sequences in the queue?

Btw, I believe this._queue.length = 0 is faster. Should probably be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. this._queue.length = 0 is faster. I'll fix. thanks.

If queue has the unexecuted query after connection.end() was called,
I think there is no need to execute surely. (unusual circumstances. such as logic error)
The opposite case is likely to be problematic.

I'll think of a better way to solve.

@ifsnow
Copy link
Contributor Author

ifsnow commented May 25, 2013

@felixge
I think that bind is more simple.
closure style was used only at a high frequency of calls. (such as getConnection, releaseConnection)
(reference : http://jsperf.com/bind-vs-closure-invocation)

@dresende
I will commit maxIdle, minIdle, maxWait feature within a few days. (while working)
I hope this is of some help to project.

@tellnes
Connection cost is fairly expensive.
Preloading of the connections can improve performance for high concurrency.
initialSize is a general option of connection pool. (ex. DBCP)

@felixge
Copy link
Collaborator

felixge commented May 26, 2013

closure style was used only at a high frequency of calls. (such as getConnection, releaseConnection)
(reference : http://jsperf.com/bind-vs-closure-invocation)

I had a look. bind() performs at 8 Mhz and regular closure at 480 Mhz. So sure it's 60x faster, but it doesn't matter unless your function is called millions of times per second. Which is clearly not the case for getConnection / releaseConnection/

Anyway, I don't really care as long as it doesn't make the code much more complicated, which shouldn't be the case here.

Connection cost is fairly expensive.

You'd have to clarify by what you mean by "expensive". MySQL is sort of optimized for a lot of connects / disconnects (because that's how a lot of PHP apps are configured to run), so it shouldn't be a problem for most cases.

Preloading of the connections can improve performance for high concurrency.

How? What's the difference between creating the connections right away vs. when it's needed? Given a busy server, both should behave identical.

@ifsnow
Copy link
Contributor Author

ifsnow commented May 27, 2013

@felixge

bind
I think it's only a difference of style. I respect your thought. I restored to bind style.

initialSize
expensive wording include remote network latency, node-mysql processing cost. description was vague (sorry)

If Pool have the usable connections before the actual requests, it's universally good.
This doesn't have the meaning which is significant in the general situation. (low traffic)
But, I think that initialSize have an effect on the response time of the initial connections after a server is restarted(deployed)

This is the result of a simple benchmark.
(It's the duration time when first 150 requests are completed.)

  • on-the-fly connections : 240ms
  • initialized connections : 5ms

@felixge
Copy link
Collaborator

felixge commented May 27, 2013

initialized connections : 5ms

I assume this does not count the pool initialization time? If not, what happens in the meantime? Clients are not being served?

@ifsnow
Copy link
Contributor Author

ifsnow commented May 27, 2013

Thank you for your interest.

It's my zero downtime deployment policy.

loop (servers)

  1. L4 Down
  2. Source Update
  3. Application Server Restart
  4. Application Server Enable Check
  5. L4 Up

I excepted the connection creation time because there was the 4 stage.
I have the version which returns a connection while a pool is initialized. but, code complexity is so high.
If you will think that this feature will be unnecessary, i will only use for the individual.

I have been working to add features such as DBCP.
http://www.tomcatexpert.com/blog/2010/04/01/configuring-jdbc-pool-high-concurrency

Enjoy the reset of your stay. :)

connection.connect(function(err) {
if (!err) {
successfulConnect++;
self._allConnectionsLength++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will not successfulConnect and self._allConnectionsLenght always be the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. This is the side-effect because of the revision. I'll remove unused variable. (Thanks!)

@felixge
Copy link
Collaborator

felixge commented May 29, 2013

I'm at jsconf right now, so I won't get to review this right away. But maybe somebody else can chime in meanwhile?

Fix : Pool.end() will not run if it is already running.
@ifsnow ifsnow closed this May 30, 2013
@ifsnow
Copy link
Contributor Author

ifsnow commented May 30, 2013

I will request again including the other features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants