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

Speed up buffer creation. #92

Merged
merged 1 commit into from
Jan 9, 2016
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,11 @@ function fromArrayBuffer (that, array) {
if (Buffer.TYPED_ARRAY_SUPPORT) {
// Return an augmented `Uint8Array` instance, for best performance
array.byteLength
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is no-op that we can remove. Just noticed it, lol

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, never mind. Heh. I remember why I added this. I'll add a comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now you have me curious :)

that = Buffer._augment(new Uint8Array(array))
that = new Uint8Array(array)
that.__proto__ = Buffer.prototype
} else {
// Fallback: Return an object instance of the Buffer class
that = fromTypedArray(that, new Uint8Array(array))
that = fromTypedArray(that, array)
Copy link
Owner

Choose a reason for hiding this comment

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

This will break if the user passes an ArrayBuffer into the Buffer object implementation. fromTypedArray takes an array-like object.

}
return that
}
Expand Down Expand Up @@ -223,6 +224,7 @@ function fromJsonObject (that, object) {

if (Buffer.TYPED_ARRAY_SUPPORT) {
Buffer.prototype.__proto__ = Uint8Array.prototype
Buffer.prototype._set = Uint8Array.prototype.set // For `copy` below.
Copy link
Owner

Choose a reason for hiding this comment

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

I know you just copied this but I wonder if we can't just use Uint8Array.prototype.set.call in copy instead of saving a reference here.

Copy link
Owner

Choose a reason for hiding this comment

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

Seems to work. I'll make the change.

Buffer.__proto__ = Uint8Array
} else {
// pre-set for values that may exist in the future
Expand All @@ -233,12 +235,11 @@ if (Buffer.TYPED_ARRAY_SUPPORT) {
function allocate (that, length) {
if (Buffer.TYPED_ARRAY_SUPPORT) {
// Return an augmented `Uint8Array` instance, for best performance
that = Buffer._augment(new Uint8Array(length))
that = new Uint8Array(length)
that.__proto__ = Buffer.prototype
} else {
// Fallback: Return an object instance of the Buffer class
that.length = length
that._isBuffer = true
}

var fromPool = length !== 0 && length <= Buffer.poolSize >>> 1
Expand Down Expand Up @@ -421,6 +422,10 @@ function slowToString (encoding, start, end) {
}
}

// Even though this property is private, it shouldn't be removed because it is
// used by `is-buffer` to detect buffer instances in Safari 5-7.
Buffer.prototype._isBuffer = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was only being set in the fallback case originally, why is it now always set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already always set (since it's added inside Buffer._augment which was always called). If it wasn't the case then Buffer.isBuffer would've failed since it only uses this property. I just moved it to the prototype to avoid having to add it everywhere.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it still added inside Buffer._augment then?

edit: Nevermind, great work @mtth, I feel like I answered my own question (emphasis: augment). Looks good!


Buffer.prototype.toString = function toString () {
var length = this.length | 0
if (length === 0) return ''
Expand Down Expand Up @@ -798,7 +803,8 @@ Buffer.prototype.slice = function slice (start, end) {

var newBuf
if (Buffer.TYPED_ARRAY_SUPPORT) {
newBuf = Buffer._augment(this.subarray(start, end))
newBuf = this.subarray(start, end)
newBuf.__proto__ = Buffer.prototype
} else {
var sliceLen = end - start
newBuf = new Buffer(sliceLen, undefined)
Expand Down Expand Up @@ -1322,6 +1328,9 @@ var BP = Buffer.prototype

/**
* Augment a Uint8Array *instance* (not the Uint8Array class!) with Buffer methods
*
* This function is also used externally by `typedarray-to-buffer`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 this is good.

*
*/
Buffer._augment = function _augment (arr) {
arr.constructor = Buffer
Copy link
Owner

Choose a reason for hiding this comment

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

I think we still need this line, or is-buffer will break.

Copy link
Owner

Choose a reason for hiding this comment

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

Nevermind. This is inherited from Buffer.prototype.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it a bit more, I think we actually need this line. Not for this package, but for typedarray-to-buffer to work with is-buffer. (This doesn't apply if you make the change you mention below about switching to setting the prototype.)

Copy link
Owner

Choose a reason for hiding this comment

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

typedarray-to-buffer is switched over now, so we don't need this line anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great!

Expand Down