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

buffer: add Buffer.createView() to create a new buffer that shares ArrayBuffer with another ArrayBufferView #29101

Closed
wants to merge 1 commit into from

Conversation

gfx
Copy link
Contributor

@gfx gfx commented Aug 13, 2019

This is a proposal for a new method Buffer.createView(), although its name could be changed.

This is not ready to merge because of missing docs and tests, but rather I've made this PR for discussion.

Rationale

When users convert ArrayBufferView (e.g. typed arrays and buffer itself) to Buffer, there are typical mistakes about handling underlying ArrayBuffer.:

  • A mistake on Buffer.from(arrayBufferView) that copies the underlying ArrayBuffer of arrayBufferView which might be unexpectedly slow
  • A mistake on Buffer.from(arrayBufferView.buffer) that creates a view of the underlying ArrayBuffer without offset and length parameters

So this PR creates a new method Buffer.createView(arrayBufferView):

const bytes = someMethodToCreateUint8Array();
// The same as `Buffer.from(bytes.buffer, bytes.byteOffset, bytes.byteLength)`
const buffer = Buffer.createView(bytes);

// It requires no temporary variable:
const buffer = Buffer.createView(someMethodToCreateUint8Array());

And also Buffer.createView(arrayBuffer) is provided as the same semantics of the current Buffer.from(arrayBuffer) to clarify that it is just a view of arrayBuffer.

I think Buffer.from(arrayBuffer) should be deprecated because it is extremely confusing, and ECMA-262's typed arrays have no such overload, anyway. However, it is not directly related to this PR.

refs: #28725

Checklist

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added NOT YET
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Aug 13, 2019
@devsnek
Copy link
Member

devsnek commented Aug 13, 2019

This seems generally useful, enough so that i'd be worried about it getting in the way of a future js language addition.

@devsnek devsnek added the wip Issues and PRs that are still a work in progress. label Aug 13, 2019
@devsnek
Copy link
Member

devsnek commented Aug 13, 2019

@nodejs/buffer

It creates a new Buffer that shares ArrayBuffer with another
ArrayBufferView. A shortcut for `Buffer.from(view.buffer,
view.byteOffset, view.byteLength)` but it requires no temporary var.

refs: nodejs#28725
@ChALkeR
Copy link
Member

ChALkeR commented Aug 13, 2019

I'm in favor, the logic looks good to me.
A significant part of direct .buffer property usage comes from the usecase described here, I believe.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 13, 2019

But as @devsnek says -- it is useful enough to be introduced to all other ArrayBuffer views.
Afaik .buffer property is also often used for "converting" Buffers to Uint*Arrays, not just for converting anything to Buffers, and this covers just one direction.

@ChALkeR ChALkeR added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 13, 2019
@gfx
Copy link
Contributor Author

gfx commented Aug 14, 2019

Actually, it would be better if ECMA-262's array buffer views have this method 😄

@Trott
Copy link
Member

Trott commented Aug 16, 2019

@nodejs/open-standards Anyone know if something like this is in the works?

@devsnek
Copy link
Member

devsnek commented Aug 16, 2019

nothing like this is currently in the works, but i am considering pursuing it at some point (not soon)

@jasnell
Copy link
Member

jasnell commented Aug 17, 2019

How would this be different from:

const arr = new Uint16Array(2);

arr[0] = 5000;
arr[1] = 4000;

// Shares memory with `arr`.
const buf = Buffer.from(arr.buffer);

console.log(buf);
// Prints: <Buffer 88 13 a0 0f>

// Changing the original Uint16Array changes the Buffer also.
arr[1] = 6000;

console.log(buf);
// Prints: <Buffer 88 13 70 17>

https://nodejs.org/dist/latest-v12.x/docs/api/buffer.html#buffer_class_method_buffer_from_arraybuffer_byteoffset_length

@ChALkeR
Copy link
Member

ChALkeR commented Aug 17, 2019

@jasnell Buffer.from(arr.buffer) depends on the way how the original view was created, i.e. if it is a view on top a larger ArrayBuffer or not.

Buffer.from(arr.buffer, arr.byteOffset, arr.byteLength) is identical to the proposed API (for the case from arr is a view), but less convenient that Buffer.createView(arr).

$ node
> const a = Buffer.from('foo')
undefined
> const b = new Uint8Array(a.buffer, a.offset, a.length)
undefined
> const c = Buffer.from(b.buffer)
undefined
> b
Uint8Array [ 102, 111, 111 ]
> c
<Buffer 66 6f 6f ef fe 7f 00 00 00 00 00 00 00 00 00 00 00 20 00  ... >

@Trott
Copy link
Member

Trott commented Aug 17, 2019

Can this be implemented as a userland module? If so, maybe that's the way to split the difference between "super useful to have right now" vs. "don't want to duplicate/conflict/whatever a future standard"?

Another possibility is to implement it but leave it as Experimental status, but I'm wary of that. People may just start using it in important modules and production systems anyway.

@benjamingr
Copy link
Member

Wouldn't it make more sense to just change .from to work correctly with ArrayBufferView?

@addaleax
Copy link
Member

Wouldn't it make more sense to just change .from to work correctly with ArrayBufferView?

Buffer.from() was intended to match new Buffer(), which in turn was supposed to match new Uint8Array() here (and new SomeTypedArray(iterable) always creates a copy). I think this works correctly, even if a bit unexpected.

I’d also expect changing the semantics of Buffer.from() to lead to subtle, unexpected breakage.

@gfx
Copy link
Contributor Author

gfx commented Dec 13, 2019

How can I make progress for this pull-request? Or if this pull-request is unacceptable, please close it.

@BridgeAR
Copy link
Member

@gfx I do not have a strong opinion on the PR itself. I am closing this, since this did not get any traction recently and it does not seem likely to get accepted as it is. Thank you very much for your work though and it does seem like it would be a good thing to publish this as a small module.

@BridgeAR BridgeAR closed this Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants