Closed catshow closed 11 years ago
I'll entertain patches but they'll have to come with benchmarks that prove there's no performance degradation.
Is this javascipt code or is it C++? Please let me know where to start and I will look at it.
@catshow It's all JavaScript. See lib/buffer.js
.
I have completed the modification to buffer.js. I will submit a pull request if these numbers suit you.
For this test I copied the buffer.js and the buffer-mod.js file into a local directory to make sure that they were doing the same thing. Then I have two scripts that require the local copies.
-- this tests the original
var Buffer = require('./buffer_orig').Buffer;
function testBuffer(){
var buffer = new Buffer(50);
var counter = 1000000;
var offset = 0;
console.time('buffer without return x 100K')
while(counter > 0){
buffer.fill(0);
offset = 0;
buffer.writeUInt8(1, offset);
offset++;
buffer.writeUInt16BE(2, offset);
offset += 2;
buffer.writeUInt32BE(3, offset);
offset += 4;
buffer.writeInt8(-1, offset);
offset++;
buffer.writeInt16BE(-2, offset);
offset += 2;
buffer.writeInt32BE(-3, offset);
offset += 4;
buffer.writeFloatBE(1.234, offset);
offset += 4;
buffer.writeDoubleBE(1.234, offset);
offset += 8;
counter--;
}
console.timeEnd('buffer without return x 100K')
}
var x = 10;
while(x-- > 0){
testBuffer();
}
------------- this tests the modified -------------------
var Buffer = require('./buffer').Buffer;
function testBuffer(){
var buffer = new Buffer(50);
var counter = 1000000;
var offset = 0;
console.time('buffer with return x 100K')
while(counter > 0){
buffer.fill(0);
offset = 0;
offset = buffer.writeUInt8(1, offset);
offset = buffer.writeUInt16BE(2, offset);
offset = buffer.writeUInt32BE(3, offset);
offset = buffer.writeInt8(-1, offset);
offset = buffer.writeInt16BE(-2, offset);
offset = buffer.writeInt32BE(-3, offset);
offset = buffer.writeFloatBE(1.234, offset);
offset = buffer.writeDoubleBE(1.234, offset);
counter--;
}
console.timeEnd('buffer with return x 100K')
}
testBuffer();
var x = 10;
while(x-- > 0){
testBuffer();
}
------------------- these are the results --------------------
$ node test/test_buffer.js buffer without return x 100K: 3869ms buffer without return x 100K: 3848ms buffer without return x 100K: 3892ms buffer without return x 100K: 3890ms buffer without return x 100K: 3869ms buffer without return x 100K: 3864ms buffer without return x 100K: 3875ms buffer without return x 100K: 3863ms buffer without return x 100K: 3865ms buffer without return x 100K: 3864ms
$ node test/test_buffer_mod.js buffer with return x 100K: 3991ms buffer with return x 100K: 3880ms buffer with return x 100K: 3921ms buffer with return x 100K: 3932ms buffer with return x 100K: 3902ms buffer with return x 100K: 3891ms buffer with return x 100K: 3901ms buffer with return x 100K: 3892ms buffer with return x 100K: 3933ms buffer with return x 100K: 3892ms buffer with return x 100K: 3888ms
As you can see they are within 30ms over 100K iterations.
I am not sure why they run so much slower than the ones compiled in with node. These are the results for running it with the compiled version of Buffer. Assuming that since the compiled version runs twice as fast, the time difference would only be 15ms over 100K iterations, using the one with a return value. I know that I would gladly sacrifice 15ms to not have to track down a bug because I messed up an offset somewhere.
node test/test_buffer.js
buffer without return x 100K: 1692ms buffer without return x 100K: 1789ms buffer without return x 100K: 1766ms buffer without return x 100K: 1760ms buffer without return x 100K: 1763ms buffer without return x 100K: 1767ms buffer without return x 100K: 1753ms buffer without return x 100K: 1768ms buffer without return x 100K: 1785ms buffer without return x 100K: 1746ms
Benchmark update, I went ahead and compiled both versions on my box and ran new tests. It looks like only 70ms over 1 million iterations total difference.
buffer without return x 100K: 1745ms buffer without return x 100K: 1749ms buffer without return x 100K: 1765ms buffer without return x 100K: 1764ms buffer without return x 100K: 1763ms buffer without return x 100K: 1768ms buffer without return x 100K: 1765ms buffer without return x 100K: 1770ms buffer without return x 100K: 1782ms buffer without return x 100K: 1757ms total time: 17635ms
buffer with return x 100K: 1797ms buffer with return x 100K: 1736ms buffer with return x 100K: 1769ms buffer with return x 100K: 1770ms buffer with return x 100K: 1771ms buffer with return x 100K: 1776ms buffer with return x 100K: 1763ms buffer with return x 100K: 1763ms buffer with return x 100K: 1781ms buffer with return x 100K: 1770ms total time: 17705ms
@catshow Can you submit your changes as a PR? If you have good benchmarks, please add them to the ./benchmark/
directory.
This change would be fairly trivial to implement. If it's still a desired feature (as in someone responds saying they'd still like it) I'll put this in my to-do list after my latest Buffer PR is cleared.
It is possible for such a change to open some possibilities of improvement for the look (and possibly even the speed) of some jDataView's internals, so I am responding here that I'd like it implemented.
While taking #5586 and #5611 into perspective, I would advocate the following.
The write methods return this
(the buffer), but the buffer itself gets a new property called offset
.
Upon initilization offset
is 0
. Every write method then saves its end to the offset
property. When a write method is called without an explicit offset as an argument, it will take the buffers offset
property as its starting point.
var buf = new Buffer(10);
console.log(buf.offset); // --> 0
buf.writeUInt8(0xFF); // no offset is explicitly specified, this.offset is used
console.log(buf.offset); // --> 1
buf.writeUInt16BE(0xAAAA); // same here
console.log(buf.offset); // --> 3
buf.writeUInt16BE(0xBBBB).writeUInt8(0xCC); // by returning this, we allow chaining
console.log(buf.offset); // --> 6
buf.writeUInt8(0xDD, 8).writeUInt8(0xEE);
console.log(buf.offset); // --> 10
console.log(buf); // <Buffer FF AA AA BB BB CC 00 00 DD EE>
I think this way, we get the best of both worlds.
However, both concerns should be treated independently. The return this;
chainability gets its own PR, as well as the offset
property.
fwiw that's what https://github.com/tjfontaine/node-buffercursor does, aside from needing to return this
on writes
@tjfontaine Ah, cool. I'll probably use that in my dhcp module.
But still, I think my proposition should be merged, as it doesn't brake anything and can vastly improve the workflow.
@silvinci I've just published a new version of buffercursor that returns this
on writes and fill, hopefully that assuages your concerns
It would be really nice to get the offset returned from all of the write* and copy methods for example:
instead of having to keep track of offset separately like this