-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Accept ArrayBuffer (and typed array/data view?) anywhere Buffer is allowed in the API #1826
Comments
I'm guessing that this would be within the confines of core and not 3rd party addons (at least such addons would need to explicitly support ArrayBuffers also)? |
@mscdex good question. I am not sure how third-party addons get access to the underlying |
@domenic if you pass Either way it can be done. This was actually on my list of things to implement, but I wanted to keep the initial PR small. TBH it wouldn't be hard to support var buf = new Buffer(16);
var ab = new ArrayBuffer(16);
buf.copy(ab); |
Third party support shouldn't be a problem. The native Buffer API still hands back the |
@trevnorris to clarify I wasn't just talking about the buffer API; I was talking about the whole io.js API surface area. FS, streams, etc. Not sure if that came across in the OP... editing now.
Given how |
Yeah definitely, can leave this for a minor version probably. |
A pointer is fine with me. If we say that an ArrayBuffer is treated as binary data then this is feasible. Typed Arrays can be a little tricky because of user's expectations (e.g. is the "length" to be written the byte length or the index length). But it can definitely be investigated. |
+1 for even talking ArrayBuffer / node.JS Buffer..
Let's say variable X = ArrayBuffer while the Y = Buffer (points to X's memory). As long as Y is alive, there is a need to keep X from GC. A naive hope / question / discussion, (although it's a clear break in the API) why not just use ArrayBuffer and node.JS buffer as it's view (optional) ? |
I would expect pointed to. All other argument types to the Buffer constructor get copied, but they're all array-like (have |
This has been inactive for a while. I'm going to throw a |
This would be more easily done now that Buffer is a Uint8Array. In fact the native ArrayBuffer would require a bit more work, but also doable now. |
It seems to me that the "right" way to fix this is to have everything internally operate on ArrayBuffers, then you have the equivalent of the following in all public binary-data accepting APIs: function f(..., buffer) {
if (ArrayBuffer.isView(buffer)) {
buffer = buffer.buffer;
// this will detect Buffers, Uint8Arrays, DataViews, etc.
}
// now you can process the ArrayBuffer buffer, maybe
// after validating that it's a true ArrayBuffer.
} |
@trevnorris Is it still on your list? If not, would you be willing to mentor someone else who wanted to do it? If so, we can add a |
A small update after #12223:
Example of converting a @Trott Regardless of @trevnorris's status, I would be available for mentoring this, both now and during the upcoming Code & Learn. If someone drafts a list of modules to convert, as far as I'm concerned even |
@Zahidul-Islam might be interested in taking this on if @TimothyGu is willing to mentor. Maybe he can start with a single module just to get a feel for it? |
@Trott Been wrapped up w/ work and async hook. If @TimothyGu is available then he should probably mentor. |
I'd like to reiterate that I'm available for mentoring. @Zahidul-Islam We can get started on any module that doesn't yet support any other TypedArrays except Uint8Array. What time will you be available for talk on IRC? I'm generally available 12am – 9am UTC due to the timezone I'm in. |
Hey @TimothyGu I've been peering at this item and wouldn't mind helping out (but could maybe make use of some mentoring). Your comment makes sense so I can probably get started, but still wouldn't mind some help. I'm at the Code&Learn today, but can also follow up later. |
PR-URL: nodejs#16042 Refs: nodejs#1826 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #16042 Refs: #1826 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@TimothyGu : maybe I'm missing something, but I couldn't find any API surface to convert to use
Also, I'm not sure how a |
Friendly reminder #22562 PR tries out the string_decoder item from the checklist. Looking forward to your review. |
Friendly reminder: #22921 tries out the |
Friendly reminder: #23210 => Simply grabbed the checklist from the comment, & appended the PR #'s to each of the modules for status tracking:
|
In tls module, accept ArrayBuffer/DataView in place of isUint8Array in the source code & related test code in "test-tls-basic-validations.js", per the "tls" item in the checklist of the comment in #1826. PR-URL: #23210 Refs: #1826 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
In tls module, accept ArrayBuffer/DataView in place of isUint8Array in the source code & related test code in "test-tls-basic-validations.js", per the "tls" item in the checklist of the comment in #1826. PR-URL: #23210 Refs: #1826 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Related change for v8 module is introduced in #23953. |
This commit allow passing `TypedArray` and `DataView` to: - v8.deserialize() - new v8.Deserializer() - v8.serializer.writeRawBytes() PR-URL: nodejs#23953 Refs: nodejs#1826 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
This commit allow passing `TypedArray` and `DataView` to: - v8.deserialize() - new v8.Deserializer() - v8.serializer.writeRawBytes() PR-URL: #23953 Refs: #1826 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #22921 Refs: #1826 Refs: #22921 (comment) Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #22921 Refs: #1826 Refs: #22921 (comment) Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
I'm eager to help @TimothyGu. If possible, I would be incredibly thankful for any guidance/mentoring. I looked at the examples you provided and think I understand what needs to be done (maybe some initial guidance?). I apologize if I come off as a complete "noob", but I really enjoy Node, the community seems great, and I want to actually CONTRIBUTE! |
This commit allow passing `TypedArray` and `DataView` to: - v8.deserialize() - new v8.Deserializer() - v8.serializer.writeRawBytes() PR-URL: #23953 Refs: #1826 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
PR-URL: #22921 Refs: #1826 Refs: #22921 (comment) Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>
Not sure if it's exactly the same problem but in my case what I want is something that is exactly the same as E.g. const UINT8_ARRAY_VIEW_SIZE = 1000 * 1000 * 500 // 500MB ?
class UncappedUint8Array extends Uint8Array {
static from(arr) {
return arr.slice();
}
constructor(arrayBuffer) {
this._arrayBuffer = arrayBuffer;
const { length } = this;
this._views = Array(Math.ceil(length / UINT8_ARRAY_VIEW_SIZE));
let offset = 0;
while (offset < length) {
const size = Math.min(length - offset, UINT8_ARRAY_VIEW_SIZE);
this._views.push(new Uint8Array(arrayBuffer, offset, size));
offset += size;
}
}
get length() {
return this._arrayBuffer.byteLength;
}
// could be used for index get
_byteAt(index) {
const viewSize = UINT8_ARRAY_VIEW_SIZE;
return this._views[Math.floor(length / viewSize)][length % viewSize];
}
// could be used for index set
_setByteAt(index, value) {
const viewSize = UINT8_ARRAY_VIEW_SIZE;
this._views[Math.floor(length / viewSize)][length % viewSize] = value;
}
indexOf(value) {
const { length } = this;
for (let i = 0; i < length; i++) {
if (this._byteAt(i) === value) {
return i;
}
}
return -1;
}
slice(startIndex, endIndex) {
return new UncappedUint8Array(this._arrayBuffer.slice(startIndex, endIndex));
}
// ... etc
} Not sure what amount of work would be required for me to implement the whole |
I’ll close this issue because I think it’s (mostly?) done. @benwiley4000 The fact that |
That is, update fs, http, streams, etc.
This is a proposal, but I would think with @trevnorris's work on making Buffer a subclass of Uint8Array, it seems likely to be not too hard...
On the web the best practice has emerged that when accepting arguments, allow any of ArrayBuffer or typed array or DataView, which is why I think more than just Buffer | ArrayBuffer would make sense.
What do people think? @trevnorris, am I understanding correctly that after your work lands this would not be very hard?
The text was updated successfully, but these errors were encountered: