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

Float16Array not working in node:v8 serde #55574

Closed
bartlomieju opened this issue Oct 28, 2024 · 6 comments · Fixed by #55996
Closed

Float16Array not working in node:v8 serde #55574

bartlomieju opened this issue Oct 28, 2024 · 6 comments · Fixed by #55996

Comments

@bartlomieju
Copy link
Contributor

Version

23.1.0

Platform

22.1.0 Darwin Kernel Version 22.1.0: Sun Oct  9 20:15:09 PDT 2022; root:xnu-8792.41.9~2/RELEASE_ARM64_T6000 arm64

Subsystem

node:v8

What steps will reproduce the bug?

import { serialize } from "node:v8";

const float16Data = new Float16Array([1.0, 2.5, 3.14]);

try {
  const serialized = serialize(float16Data);
  console.log("Serialization successful!");
  console.log("Serialized data:", serialized);
} catch (error) {
  console.error("Serialization failed:", error.message);
}

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior? Why is that the expected behavior?

serialize calls finishes successfuly.

What do you see instead?

const float16Data = new Float16Array([1.0, 2.5, 3.14]);
                    ^

ReferenceError: Float16Array is not defined
    at file:///Users/ib/dev/deno/index.js:3:21

Additional information

While I understand that Float16Array is not yet supported (eg. #52416) as it requires upgrade to V8 12.4, maybe there's a chance we could agree that arrayBufferViewTypeToIndex will return 13 for the Float16Array?

node/lib/v8.js

Lines 277 to 293 in 5633c62

function arrayBufferViewTypeToIndex(abView) {
const type = ObjectPrototypeToString(abView);
if (type === '[object Int8Array]') return 0;
if (type === '[object Uint8Array]') return 1;
if (type === '[object Uint8ClampedArray]') return 2;
if (type === '[object Int16Array]') return 3;
if (type === '[object Uint16Array]') return 4;
if (type === '[object Int32Array]') return 5;
if (type === '[object Uint32Array]') return 6;
if (type === '[object Float32Array]') return 7;
if (type === '[object Float64Array]') return 8;
if (type === '[object DataView]') return 9;
// Index 10 is FastBuffer.
if (type === '[object BigInt64Array]') return 11;
if (type === '[object BigUint64Array]') return 12;
return -1;
}

Thanks!

@avivkeller avivkeller added web-standards Issues and PRs related to Web APIs v8 engine Issues and PRs related to the V8 dependency. and removed v8 engine Issues and PRs related to the V8 dependency. web-standards Issues and PRs related to Web APIs labels Oct 28, 2024
@avivkeller
Copy link
Member

avivkeller commented Oct 28, 2024

This doesn't have anything to do with the serialize function. Node.js doesn't provide a Float16Array:

$ node -p "Float16Array"                                     
[eval]:1
Float16Array
^

ReferenceError: Float16Array is not defined

@bartlomieju
Copy link
Contributor Author

Well, yeah, at the moment it doesn't. But once Float16Array is added I think the support in node:v8 will follow?

@richardlau
Copy link
Member

richardlau commented Oct 28, 2024

As noted in #52416 (comment), support for Float16Array is in-progress in V8. It's currently behind a --js-float16array V8 runtime flag.

@bnoordhuis
Copy link
Member

Hiya, Bartek.

maybe there's a chance we could agree that arrayBufferViewTypeToIndex will return 13 for the Float16Array?

Seems uncontroversial to me. The one thing is that arrayBufferViewIndexToType(13) is going to fail with a "Float16Array is not defined" ReferenceError but everything else will work just fine though.

ashvardanian added a commit to ashvardanian/SimSIMD that referenced this issue Nov 3, 2024
NodeJS type system doesn't define `float16`,
so we don't currently have a way of detecting
such arrays.

In Python bindings we allow passing an additional
string parameter together with `uint16` arrays to
override the logical type for the missing `bfloat16`.
In JS, for now, it's wiser to wait for the NAPI to
define the missing `napi_typedarray_type` value.

nodejs/node#55574 (comment)
nodejs/node#52416 (comment)
@bartlomieju
Copy link
Contributor Author

Hiya, Bartek.

maybe there's a chance we could agree that arrayBufferViewTypeToIndex will return 13 for the Float16Array?

Seems uncontroversial to me. The one thing is that arrayBufferViewIndexToType(13) is going to fail with a "Float16Array is not defined" ReferenceError but everything else will work just fine though.

Hey Ben, thanks for commenting. Should I open a PR that updates these function already? As you said they would fail with ReferenceErrors for now, but once Float16Array support lands they would start working.

@targos
Copy link
Member

targos commented Nov 4, 2024

I guess we can update the functions and add the --js-float16array flag in the test to validate it works.

aduh95 pushed a commit that referenced this issue Dec 10, 2024
PR-URL: #55996
Fixes: #55574
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
ruyadorno pushed a commit that referenced this issue Dec 20, 2024
PR-URL: #55996
Fixes: #55574
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants