Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Feb 27, 2015

I was looking for ways to speed up mysql.format after noticing that the performance was very slow when attempting to format a query with 100,000+ values. I came up with a couple of improvements, by far the most significant is to use an iterator to step through the values array instead of Array.shift(). See: http://jsperf.com/some-array-reading-comparisons/6

I was looking for ways to speed up mysql.format after noticing that the performance was very slow when attempting to format a query with 100,000+ values. I came up with a couple of improvements, but by far the most significant is using an iterator to step through the values array instead of Array.shift(). See: http://jsperf.com/some-array-reading-comparisons/6
@ghost
Copy link
Author

ghost commented Feb 27, 2015

@ghost
Copy link
Author

ghost commented Feb 27, 2015

May fix #937

@dougwilson
Copy link
Member

This sounds good to me, but please make sure that the values array stays altered in the same way in the end as it used to be; there are unfortunately people relying on the fact that the array they are passing in gets emptied (or only emptied up to the number of ? in the SQL).

@dougwilson
Copy link
Member

Or nevermind my statement above; :) I'll doube-check that for realz and most likely I'll merge your code :O

@dougwilson dougwilson self-assigned this Feb 27, 2015
@ghost
Copy link
Author

ghost commented Feb 27, 2015

Hold off on that merge. With this change my test suite for my application is failing. Could be an obscure use case. I will update here after I sort it out.

Never mind sentence above. It had nothing to do with this change, I was mistaken.

@dougwilson dougwilson added this to the 2.6 milestone Mar 8, 2015
@dougwilson dougwilson closed this in f39856c Mar 8, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for array copying, as you're not modifying it anymore. More win :)

Copy link
Member

Choose a reason for hiding this comment

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

@ronkorving feel free to make a PR :) And remember that the values argument may not always be an array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch, I missed that it can be an object too. Then there's nothing to gain here. My bad.

seangarner pushed a commit to seangarner/node-mysql that referenced this pull request May 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

3 participants