Buffer implemented using Uint8Array

[trevnorris]

Creating this issue so the following can be discussed in the TC meeting:

1. Deciding whether Uint8Array backed Buffer can roll out on a minor release or only on a major.
2. My plan is to release the new functionality behind a flag for v3.0. This will allow a full release cycle to test the new functionality before it has to be rolled out by default in v4.0.
3. Can we get numbers on how many packages are using V8 APIs like v8::Object::GetIndexedPropertiesExternalArrayData()?
4. Can someone check V8 to see why new ArrayBuffer() is limited to 0x3fffffff? (same as Buffer) If the memory is pre-allocated (which we will be doing) then there is no upper limit. This discrepancy only happens in 4.3. In 4.4 it's fixed so both APIs can be arbitrarily large.
5. The new Buffer implementation will no longer be setting the class id's. Since they are not actually Persistent's anymore. Will this affect anyone?

[kkoopa]

If Buffer::New can return an empty handle, it should be changed to return a MaybeLocal<Object>.

2: If there will be breaking changes to the API, it should not be hidden behind flags. Better be preemptive and avoid additional woes for the next update.

4: Probably because of https://github.com/nodejs/io.js/blob/next/src/node.cc#L168 and https://github.com/nodejs/io.js/blob/next/src/node.cc#L177

[trevnorris]

@kkoopa Using MaybeLocal<Object> does seem to be the best choice. I'll make that change a separate commit in my PR.

I have greater concern for security than for API compatibility. Buffer underpins almost everything, and so I believe we should be cautious about a new implementation. The point of the flag is to allow a release cycle of the new implementation being publicly available for testing, bug filing, etc. before it's rolled out as standard.

Great find on (4). Since ArrayBuffer no longer has that same limitation we should roll back that logic.

[kkoopa]

I understand why you may want to keep it behind a flag, but keep in mind that nobody uses flags or release candidates. Having it there behind a flag is almost like not having it at all. Who would
a) know about this flag
b) decide to enable it
apart from you and me?

Any new major release is expected to have bugs which will be fixed in subsequent patches. As I understand it, the JS API will be largely unaffected by the change and those using the C++ API will have a lot of rewriting to do anyway, because of the changes in V8. NAN does not detect runtime flags, and so could not know which one is in use and would default to the non-flagged version. This will result in the new API not getting used or tested before io.js 4.0, when they will have to update their code again, having just fixed a bunch of stuff for 3.0.

Regarding point number 3: I made a version conversion script for the NAN 1 -> 2 transition, which will warn about those APIs being removed, saying the code needs to be rewritten using Buffer. Actually finding how many pacakges use these APIs is probably not doable. Closest I can think of is doing a code search on GitHub. This lists 3690 results, many of which are false positives and duplicates.

[trevnorris]

The runtime flag makes switching to the new implementation transparent from both the JS and C++ side. As for the new Buffer API returning a MaybeLocal, and accepting a size_t instead of a uint32_t, those will be in place. So regardless of using the flag, users will have to update their code to match.

Thanks for looking into (3). Nice to know that developers have generally stayed away from using that part of the V8 API directly.

[brendanashworth]

Question (forgive the incompetence): does this mean Buffer will no longer be allocated with malloc and friends, and does this mean the Buffer implementation will be mostly JS?

[trevnorris]

@brendanashworth It still uses malloc under the hood. Plus, we'll be using the native ArrayBuffer API extensively with pre-allocated memory and such. Fundamentally Buffer will function very similar to how it did before.

[trevnorris]

Fixed by 65cd82a and subsequent commits.