nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.78k stars 29.68k forks source link

`Float16Array` not working in `node:v8` serde #55574

Open bartlomieju opened 2 weeks ago

bartlomieju commented 2 weeks ago

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. https://github.com/nodejs/node/issues/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?

https://github.com/nodejs/node/blob/5633c62219e199baac833e8862d60333d85dc3d3/lib/v8.js#L277-L293

Thanks!

RedYetiDev commented 2 weeks ago

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 commented 2 weeks ago

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

richardlau commented 2 weeks ago

As noted in https://github.com/nodejs/node/issues/52416#issuecomment-2043448555, support for Float16Array is in-progress in V8. It's currently behind a --js-float16array V8 runtime flag.

bnoordhuis commented 2 weeks ago

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.

bartlomieju commented 1 week ago

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 commented 1 week ago

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