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

Investigate proper instanceof Buffer support #40

Closed
feross opened this issue Aug 10, 2014 · 7 comments
Closed

Investigate proper instanceof Buffer support #40

feross opened this issue Aug 10, 2014 · 7 comments
Labels

Comments

@feross
Copy link
Owner

feross commented Aug 10, 2014

I saw a potential way to get instanceof Buffer working in a module by @dcodeIO: https://github.com/dcodeIO/node-BufferView/blob/master/BufferView.js

    // Extend DataView and clear all instance methods to be sure
    BufferView.prototype = Object.create(DataView.prototype);
    (function() { for (var i in BufferView.prototype) delete BufferView.prototype[i]; })();
@dcodeIO
Copy link

dcodeIO commented Aug 11, 2014

Unfortunately, the delete statement there usually does nothing, as all of the properties exist deeper in the prototype chain. It's literally just there for the case that there is one.

From MDN:

If the object inherits a property from a prototype, and doesn't have the
property itself, the property can't be deleted by referencing the object.
You can, however, delete it directly on the prototype.
function Foo(){}
Foo.prototype.bar = 42;
var foo = new Foo();

// returns true, but with no effect, 
// since bar is an inherited property
delete foo.bar;           

@feross
Copy link
Owner Author

feross commented Oct 29, 2014

I pushed an attempt to fix instanceof to this branch: https://github.com/feross/buffer/tree/fix-instanceof

@feross
Copy link
Owner Author

feross commented Oct 29, 2014

The instanceof fix I pushed seems to work, but there's a big performance hit in non-V8 engines. This is unacceptable, so I won't be merging it. I'll leave it here in case anyone has ideas for how to proceed.

@feross
Copy link
Owner Author

feross commented Mar 12, 2015

New possible solution: nodejs/node#106 (comment)

@feross
Copy link
Owner Author

feross commented Sep 16, 2015

instanceof Buffer works correctly in buffer v3.5.0 now!

This fixes the last remaining incompatibility with the node Buffer! And since Buffer in node v4.0.0 is based on Uint8Array as well, we're on exactly the same page now!

There's a 50% performance hit on the 16 byte buffer test (new Buffer(16)) in non-V8 engines (much better than my previous experiments to get this working). For the 16KB large buffer test (new Buffer(1600)), the hit is only 25%, since the difference is drowned out by the zeroing out operation. I expect this to get better over time since subclassing arrays is supported by ES6 now. 👍

@feross feross closed this as completed in 1d20f50 Sep 16, 2015
@dcousens
Copy link
Collaborator

@feross any reason why node 4.0.0 couldn't be using the same Buffer code as this library? (hell, why not the same library).

@feross
Copy link
Owner Author

feross commented Sep 16, 2015

Well, they do a bunch of calls directly into C++. I think one reason they do that is so they can disable the behavior where new typed array memory must be zeroed-out for security reasons in the browser environment. In node, they can do without that and get perf improvements.

Maybe there are more reasons...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants