Skip to content

Commit

Permalink
Merge pull request #139 from rvagg/read-stream-performance
Browse files Browse the repository at this point in the history 
50% performance increase in ReadStream by removing one bind.
  • Loading branch information
@mcollina
mcollina committed May 16, 2013
2 parents 15b4ad4 + 76f9ab9 commit 63399d0
Show file tree
Hide file tree
Showing 7 changed files with 65 additions and 3 deletions.
7 changes: 5 additions & 2 deletions lib/read-stream.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,15 +117,18 @@ ReadStream.prototype.pipe = function (dest) {
}

ReadStream.prototype._read = function () {
var that = this
if (this._state.canRead()) {
this._state.read()
this._iterator.next(this._onData.bind(this))
this._iterator.next(function(err, key, value) {

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton May 16, 2013

You can optimize this further!

var reThis = /\bthis\b/,
    fnToString = Function.prototype.toString;

// ...then later, inside `_read`
if (this._state.canRead()) {
  var that = this
  this._state.read()
  this._iterator.next(
    reThis.test(fnToString.call(that._onData))
      ? function(err, key, value) { that._onData(err, key, value) }
      : that._onData
  );
}

The gist is if the onData callback doesn't reference this then it doesn't get wrapped.

This comment has been minimized.

Sign in to view

Copy link
@Raynos

Raynos May 16, 2013

Member

Is that safe? What about eval? What about ES6? Feels like magic.

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton May 16, 2013

It's leveraging the common behavior of Function.prototype.toString (less worrisome as the only environment is V8). Also, good news, ES6 doesn't specify a change in the existing behavior. What's great about this approach is that false positives just get the normal wrap treatment, so not so bad. This also avoid binding pre-bound functions as they coerce to function () { [native code] } \o/. It's a good way to back-in perf for systems designed with implicit binding.

This comment has been minimized.

Sign in to view

Copy link
@juliangruber

juliangruber May 16, 2013

Member

V8 isn't the only environment, with level.js and other backends levelup will be used in all the different browsers.

Do you have numbers how much this would gain?

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton May 16, 2013

V8 isn't the only environment, with level.js and other backends levelup will be used in all the different browsers.

That's fine, its a defacto standard as well (with no gap in coverage).

Do you have numbers how much this would gain?

I can't run the benchmarks atm so feel free to fire it up and see.

This comment has been minimized.

Sign in to view

Copy link
@mcollina

mcollina May 16, 2013

Author Member

The _onData function does reference this, so it won't get any better, right?

What we can do is to generate a pre-wrapped callback for _onData.

I just don't know if this makes sense: creating a function should be cheap.
The optimization race should end somewhere.

This comment has been minimized.

Sign in to view

Copy link
@dominictarr

dominictarr May 16, 2013

Contributor

This isn't necessary, because there is only one value for this._onData. it's unique to the stream instance, it's not set by the user.
So, actually running this will only ever trigger the first case.

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton May 16, 2013

Where do the on("data", ...) handlers get executed? This would help them.

This comment has been minimized.

Sign in to view

Copy link
@jdalton

jdalton May 16, 2013

For reference here is the technique applied to another event system:
http://jsperf.com/events-vs-events2/33#chart=bar.
that._onData(err, key, value)
})
}
}

ReadStream.prototype._onData = function (err, key, value) {
this._state.endRead()
if (err || !arguments.length /* end */ || !this._state.canEmitData())
if (err || (key === undefined && value === undefined) || !this._state.canEmitData())
return this._cleanup(err)
this._read() // queue another read even tho we may not need it
try {
Expand Down
32 changes: 31 additions & 1 deletion test/benchmarks/tests/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,34 @@ module.exports = {
, 'LevelUP (no Snappy)' : require('./batch_int_string_x100000_levelup')
, 'Leveled' : require('./batch_int_string_x100000_leveled')
}
}

, 'readStream x 10': {
'LevelUP' : require('./readStream_x10_levelup')
, 'LevelUP (release)' : require('./readStream_x10_levelup')
, 'LevelUP (no Snappy)' : require('./readStream_x10_levelup')
}

, 'readStream x 100': {
'LevelUP' : require('./readStream_x100_levelup')
, 'LevelUP (release)' : require('./readStream_x100_levelup')
, 'LevelUP (no Snappy)' : require('./readStream_x100_levelup')
}

, 'readStream x 1,000': {
'LevelUP' : require('./readStream_x1000_levelup')
, 'LevelUP (release)' : require('./readStream_x1000_levelup')
, 'LevelUP (no Snappy)' : require('./readStream_x1000_levelup')
}

, 'readStream x 10,000': {
'LevelUP' : require('./readStream_x10000_levelup')
, 'LevelUP (release)' : require('./readStream_x10000_levelup')
, 'LevelUP (no Snappy)' : require('./readStream_x10000_levelup')
}

, 'readStream x 100,000': {
'LevelUP' : require('./readStream_x100000_levelup')
, 'LevelUP (release)' : require('./readStream_x100000_levelup')
, 'LevelUP (no Snappy)' : require('./readStream_x100000_levelup')
}
}
17 changes: 17 additions & 0 deletions test/benchmarks/tests/readStream_x100000_levelup.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@

var setupFn = function (count, db, cb) {
var doWrites = function() {
if(--count === 0) return cb()
db.put("aa" + count, "bb" + count, doWrites)
}
doWrites()
}

, fn = function (db, cb) {
db.createReadStream().on("end", cb)
}

module.exports = fn
module.exports.fn = fn
module.exports.setup = setupFn.bind(null, 100000)
module.exports.setupFn = setupFn
3 changes: 3 additions & 0 deletions test/benchmarks/tests/readStream_x10000_levelup.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@

module.exports = require("./readStream_x100000_levelup").fn.bind(null)
module.exports.setup = require("./readStream_x100000_levelup").setupFn.bind(null, 10000)
3 changes: 3 additions & 0 deletions test/benchmarks/tests/readStream_x1000_levelup.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@

module.exports = require("./readStream_x100000_levelup").fn.bind(null)
module.exports.setup = require("./readStream_x100000_levelup").setupFn.bind(null, 1000)
3 changes: 3 additions & 0 deletions test/benchmarks/tests/readStream_x100_levelup.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@

module.exports = require("./readStream_x100000_levelup").fn.bind(null)
module.exports.setup = require("./readStream_x100000_levelup").setupFn.bind(null, 100)
3 changes: 3 additions & 0 deletions test/benchmarks/tests/readStream_x10_levelup.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@

module.exports = require("./readStream_x100000_levelup").fn.bind(null)
module.exports.setup = require("./readStream_x100000_levelup").setupFn.bind(null, 10)

0 comments on commit 63399d0

Please sign in to comment.