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

Optimize escaping SQL #1331

Closed
wants to merge 1 commit into from
Closed

Conversation

nwoltman
Copy link
Contributor

@nwoltman nwoltman commented Jan 14, 2016

In today's V8, appending to a string is faster than building an array and calling .join().

Benchmark

$ npm install benchmark

'use strict';

var Benchmark = require('benchmark');
var SqlString = require('mysql/lib/protocol/SqlString');

var ids = ['id', 'name'];
var arrays = [['value', 12]];
var object = {
  id: 1,
  name: 'Some Name',
  email: 'somename@email.com',
  parentId: 20,
  notify: true,
};

new Benchmark.Suite()
  .add('escapeId', () => {
    SqlString.escapeId(ids);
  })
  .add('escape arrays', () => {
    SqlString.escape(arrays);
  })
  .add('escape object', () => {
    SqlString.escape(object);
  })
  .on('cycle', function(event) {
    console.log(String(event.target));
  })
  .run();

Results:

Test Before (ops/sec) After (ops/sec) Speedup
escapeId 1,260,802 1,771,151 40% (1.4x)
escape arrays 1,037,337 2,384,038 130% (2.3x)
escape object 279,876 456,599 63% (1.63x)

In today's V8, appending to a string is faster
than building an array and calling .join()
}).join(', ');
var sql = '';
for (var i = 0; i < val.length; i++) {
sql += (i ? ', ' : '') + escapeId(val[i], forbidQualified);
Copy link
Member

Choose a reason for hiding this comment

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

The change from SqlString.escapeId to just escapeId will affect people who are using AOP around this function, since we have been calling it as a method. Just noting this if you wonder why that part is reverted in the merge of your pull request.

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.

None yet

2 participants