Speed up buffer creation. #92

mtth · 2016-01-08T06:44:19Z

Since 1d20f50 (#40), instances already gained all the Buffer's
methods via prototype re-assignment. Skipping the _augment
call yields significant speedups:

~50% faster small buffer creation:
- new(16) before x 595,757 ops/sec ±0.92% (91 runs sampled)
- new(16) after x 894,462 ops/sec ±0.69% (87 runs sampled)
~20% faster slice:
- slice before: 2,176,194 ops/sec ±1.59% (89 runs sampled)
- slice after: 2,624,209 ops/sec ±0.42% (99 runs sampled)

Other benchmarks aren't significantly different. (These numbers are for v8.)

Also removed the similarly redundant _isBuffer property. In case you'd
like to keep it (even though it's private), let me know.

dcousens · 2016-01-08T07:44:22Z

LGTM, but I think @feross would be the most knowledgeable about all the subtleties for this.

mtth · 2016-01-08T16:47:21Z

Thanks @dcousens !

@feross - I'm also curious whether you would consider adding an option to not rely on prototype mutation (falling back to "augmenting" array instances). Without it, buffers are still very slow in some (most?) engines. For example here are results for Firefox 43:

BrowserBuffer#bracket-notation x 487,929 ops/sec ±1.33% (59 runs sampled) # before
BrowserBuffer#bracket-notation x 494,476 ops/sec ±1.19% (66 runs sampled) # after
Uint8Array#bracket-notation x 24,571,591 ops/sec ±1.12% (65 runs sampled) # !

I initially thought that clients would just be able to set window.TYPED_ARRAY_SUPPORT to false in these cases, but it feels a bit like a hack (since these browsers do support it), and it's slower (especially for slices, which then require a copy).

If this sounds reasonable to you, I'll update this pull request with a tentative implementation.

feross · 2016-01-08T17:15:53Z

We can't get rid of the _isBuffer property since that's needed for is-buffer (https://github.com/feross/is-buffer) to work in older Safari. is-buffer is used by browserify whenever there's a Buffer.isBuffer call (very common) to save requiring the whole buffer module in browser bundles.

Removing _augment will break typedarray-to-buffer (https://github.com/feross/typedarray-to-buffer) but that can be fixed by moving _augment directly into that package.

I'm also curious whether you would consider adding an option to not rely on prototype mutation (falling back to "augmenting" array instances)

I don't want to expose any options that aren't in the node buffer, because then we'll have code that's browser-specific and could end up doing something different in future versions of node. We should solve the issue directly in this module, but that should be done in another issue/PR, IMO.

mtth · 2016-01-08T17:37:49Z

We can't get rid of the _isBuffer property since that's needed for is-buffer (https://github.com/feross/is-buffer) to work in older Safari.

Sounds good. I'll add the _isBuffer property back.

Removing _augment will break typedarray-to-buffer (https://github.com/feross/typedarray-to-buffer) but that can be fixed by moving _augment directly into that package.

I'll restore _augment as well then. (It might also come in handy if some fallback mechanism is implemented.)

I don't want to expose any options that aren't in the node buffer, because then we'll have code that's browser-specific and could end up doing something different in future versions of node. We should solve the issue directly in this module, but that should be done in another issue/PR, IMO.

I'm fine doing the detection inside this library (though there's the risk of being suboptimal for future browser implementations). Also, maybe I didn't explain myself correctly, I didn't mean an option at instantiation time but rather something like what TYPED_ARRAY_SUPPORT currently does.

Since 1d20f50 (#40), instances already gained all the `Buffer`'s methods via the prototype re-assignment. Skipping the `_augment` call yields significant speedups: ~50% faster small buffer creation: new(16) before: 595,757 ops/sec ±0.92% (91 runs sampled) new(16) after: 894,462 ops/sec ±0.69% (87 runs sampled) ~20% faster slice: slice before: 2,176,194 ops/sec ±1.59% (89 runs sampled) slice after: 2,624,209 ops/sec ±0.42% (99 runs sampled) Other benchmarks aren't significantly different.

dcousens · 2016-01-09T06:34:53Z

index.js

@@ -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`.


👍 this is good.

feross · 2016-01-09T13:46:15Z

I can fix up the issues and merge. I'll add new tests to catch the issues I manually caught.

feross · 2016-01-09T13:56:17Z

index.js

@@ -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`.
+ *
 */
 Buffer._augment = function _augment (arr) {
  arr.constructor = Buffer


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

Nevermind. This is inherited from Buffer.prototype.

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.)

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

feross · 2016-01-09T15:36:39Z

We can actually just remove Buffer._augment entirely. I'll update typedarray-to-buffer to do just modify __proto__ like package now does.

feross · 2016-01-09T17:36:45Z

Released as v4.1.0.

mtth · 2016-01-09T18:01:37Z

Thanks @feross @dcousens !

feross · 2016-01-09T18:44:41Z

This is released in browserify v13: https://github.com/substack/node-browserify/blob/master/changelog.markdown

dcousens · 2016-01-10T02:59:08Z

Awesome work @mtth / @feross 👍

dcousens assigned feross Jan 8, 2016

dcousens added the performance label Jan 8, 2016

dcousens reviewed Jan 9, 2016
View reviewed changes

feross reviewed Jan 9, 2016
View reviewed changes

feross merged commit 12a3318 into feross:master Jan 9, 2016

feross added a commit that referenced this pull request Jan 9, 2016

fix buf introduced in PR #92

7ab2cf7

feross mentioned this pull request Jan 9, 2016

buffer@4 browserify/browserify#1476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up buffer creation. #92

Speed up buffer creation. #92

mtth commented Jan 8, 2016

dcousens commented Jan 8, 2016

mtth commented Jan 8, 2016

feross commented Jan 8, 2016

mtth commented Jan 8, 2016

dcousens Jan 9, 2016

feross commented Jan 9, 2016

feross Jan 9, 2016

feross Jan 9, 2016

mtth Jan 9, 2016

feross Jan 9, 2016

mtth Jan 9, 2016

feross commented Jan 9, 2016

feross commented Jan 9, 2016

mtth commented Jan 9, 2016

feross commented Jan 9, 2016

dcousens commented Jan 10, 2016

Speed up buffer creation. #92

Speed up buffer creation. #92

Conversation

mtth commented Jan 8, 2016

dcousens commented Jan 8, 2016

mtth commented Jan 8, 2016

feross commented Jan 8, 2016

mtth commented Jan 8, 2016

dcousens Jan 9, 2016

Choose a reason for hiding this comment

feross commented Jan 9, 2016

feross Jan 9, 2016

Choose a reason for hiding this comment

feross Jan 9, 2016

Choose a reason for hiding this comment

mtth Jan 9, 2016

Choose a reason for hiding this comment

feross Jan 9, 2016

Choose a reason for hiding this comment

mtth Jan 9, 2016

Choose a reason for hiding this comment

feross commented Jan 9, 2016

feross commented Jan 9, 2016

mtth commented Jan 9, 2016

feross commented Jan 9, 2016

dcousens commented Jan 10, 2016