make buffer[index] work #14

Merged
merged 19 commits into from Mar 29, 2013

5 participants

@substack

This unfortunately took a lot of commits since buffer[index] only works with the underlying SlowBuffer and not with Buffer, so I had to merge all the method signatures from Buffer over top of the SlowBuffer implementations.

Despite the sheer volume of changes, all the existing tests pass plus some new ones. After I got all the tests to pass in node, I inlined all the comparisons with the built-in Buffer so that we can run the test suite in browsers.

I also included a "testling" field for the package.json so we can verify which browsers are currently working and which ones are failing.
Here's the status for my fork: https://ci.testling.com/substack/buffer-browserify

To add a hook for testling, add http://git.testling.com to the "WebHook URLs" section of the server hooks page. Once the hook is working you can add a badge to the readme with:

[![browser compatibility](https://ci.testling.com/toots/buffer-browserify.png)](https://ci.testling.com/toots/buffer-browserify)
@substack substack referenced this pull request Mar 20, 2013
Open

node.js simple tests #1

@toots
Owner

Thank you so much! I will review and merge this very soon!!

@substack

We're having some issues with testling-ci right now but I ran the test suite locally in chrome and it was completely passing.

@tonistiigi

Just wanted to make a comment that by adding this we (IMHO) are removing the possibility to ever support typed arrays as a backing for the buffers. I think there is no way to subclass Uint8Array. (In browsers that support __proto__ it could be done for read only mode).

The issue I'm having:

  • User selects file with the input field
  • I want to use some node module on the data or just upload with request so I convert Blob to Buffer(very slow for loop copy to dynamic array)
  • I upload it with request. In http-browserify internals the buffer is again converted from dynamic array to a typed array.
  • So I get 2 slow copies(freezes browser even on small files) when actually no copies are needed at all because all of them could use same ArrayBuffer as a backing.

I was planning to rewrite current SlowBuffer implementation so it would switch to Uint8Array when possible and not make copies when not needed.

Or are there better alternatives to make buffer-browserify play nice with the binary formats already present in the browser? I realize that having index accessors it also important.

@substack I don't get the slicing? It just sets the offset/length property but doesn't even keep the reference to the subject(parent)?

@substack

@tonistiigi The problem with re-encoding is present in node when people use typed arrays too. buffer-browserify is just node's Buffer API which does wrap around like a Uint8Array would do. That said, I think it would be much easier to update the patched code here to start using Uint8Array than to use the original code since in the original code you'll need to juggle Buffer and SlowBuffer api surface area instead of just Buffer.

@tonistiigi

@substack

The difference of course is that in Node you can use Buffer for all your binary needs but in the browser there are many APIs that know nothing about buffers and use typed arrays instead.

I though about it a bit more and think my problem should have a better solution by just adding 2 new methods Buffer.fromUint8Array() and Buffer.prototype.toUint8Array() (or bufferToUint8Array(b) etc). And I think they should be included with browserify even though they don't exists in Node because things like http-browserify should use them.

The slicing issue still remains. Currently it doesn't work at all like in Node.

@substack

A separate module could handle the {from,to}Uint8Array logic since that would work in both node and browsers. This patch doesn't keep a reference to the parent because that doesn't work with indexing. I don't think I've ever seen any code that depends on the buffer.parent internals except for the buffer implementation itself.

@tonistiigi

I'm not taking specifically about the need have a reference to parent. With this patch when you call slice() for the buffer the returning buffer does not have any data associated with it. You can't use the index properties or read* methods for that that buffer.

@toots
Owner

Hi,

Just read the PR diff and tried the tests. It's unfortunate that there is so much diff but it seems to be working fine.

Concerning the issue with typed arrays and html-specific javascript APIs for binary blobs, I agree with @substack , it's not something that can or should be tackled here. Having a Buffer API that works is already good enough.

Mixing with HTML5 APIs is another beast that'd require much more though and a bigger project. It could go either way, i.e. having a node implementation of HTML5 file and blob API or having an optional Buffer implementation using HTML5 API in buffer-browserify .. Makes my head spin :-)

I'll have another pass at the PR and merge it later tonight or tomorrow if all is still well..

@tonistiigi tonistiigi added a commit to tonistiigi/buffer-browserify that referenced this pull request Mar 25, 2013
@tonistiigi tonistiigi Add simple failing slice test to #14 2eb4d47
@juliangruber

just stumbled over this today, big +1 from me.

@toots toots merged commit b9492bf into toots:master Mar 29, 2013
@tonistiigi

I don't really get why everyone is ignoring the fact that buffer.slice() doesn't work any more. This PR brakes every piece of code that I have ever written that uses buffer-browserify.

@toots
Owner

It's not being ignore on my side, just not implemented yet with the new buf[i]. buffer-browserify hasn't been released yet for this reason. PR welcome btw :)

@chrisdickinson

this actually worries me a lot, perf-wise. it changes the semantics of .slice from "no copy" to "full copy", which will dramatically affect a lot of the libs I've been writing.

@toots
Owner

Alright guys, I need to make a decision real quick because other people are starting to work on the master branch, even though it's not been released yet.

I personally believe that performance has never been an important choice because the goal of the module is to be as compatible as possible with node's original API, regardless of performances.

It seems unreasonable to me to write node code, wrap it for the browser and expect it to have competitive performances there... Low-level APIs, like file access, etc.. are too different in my opinion to grant this in general.

Thus, I'm tempted to move on with the current support for buf[i] add a slow slice and call it a day...

I'm open to be convinced otherwise, tho.

@tonistiigi

Its not only performance. In Node the memory is shared so if you change one the other one will also change. This can't be achieved with a copy.

@tonistiigi tonistiigi added a commit to tonistiigi/buffer-browserify that referenced this pull request Apr 12, 2013
@tonistiigi tonistiigi Add another slice test to #14 ae792a9
@toots
Owner

Hi,

Been reviewing the code wrt to previous behaviour of the slice function and it appears that slice was indeed working as in node before, by sharing data between the original Buffer and the sliced one.

I also think that it is not possible to implement this behaviour in the code after merging this PR. I'd like to be proven wrong tho.

Consequently, my conclusion is that the this[i] API is mutually exclusive with a proper slice implementation. As such, I think slice is kinda important and, regarding this[i], I'd rather have no API at all that than an API that either diverges from node's behavior or does not implement slice.

Thus, I am tempted to revert the merge of #14.

Let me know what you guys think..?

@substack

Why isn't this on npm?

@toots
Owner

@substack because I am not sure that this is a good idea. What is your thinking on the slice issue discussed above?

@toots
Owner

Ok. After much delay and some thinking, I'll release with these chances and no slice implemented (for now). Seems to be that the likelihood of programmers from node using buf[i] is much greater than the likelihood of programmers expecting changes in slice'd buffer to be reflected in original buffer. Will prolly add slice soon as well.

@tonistiigi

@toots I would suggest you to also merge my failing tests so people can see what currently works and what doesn't. Maybe solution is found and these start to pass. Or slice() should throw an error to let the user know that this is not something that can be used any more.

btw, regarding the speed issues I discussed before I have meanwhile written a module that solves that for me https://github.com/tonistiigi/uint8 . Too bad it will not work in this updated version.

@andris9 andris9 referenced this pull request in andris9/mimelib Oct 3, 2013
Closed

Deprecation warning on node 0.11.6 #23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment