bug: encodeInto implementation is not correct #9006
Comments
lucacasonato
added
bug
|
I set up a quick test using a detached buffer and Deno threw an exception,
const memory = new WebAssembly.Memory({
initial: 1,
maximum: 1,
shared: false
});
const array = new Uint8Array(memory.buffer, 0, 100);
memory.grow(0);
const result = new TextEncoder().encodeInto("Hello, world!", array);
console.log(array, result);
Is this even related to the WPT test assertion? I've updated Deno's |
ghost
mentioned this issue
|
#9143 did not fix the error WPT errors. |
|
Unfortunate. Can I get more details on this WPT test in particular? i.e. where are the WPT tests sourced? |
|
Can't you run it? uncomment / comment the tests you care about in //cli/tests/wpt.jsonc, then run |
|
Checking out the actual test, the problem isn't what it seems at all, as far as I can tell, Deno's implementation is okay. Besides that Deno is missing If the test could've successfully detached the buffer, then it would've encountered the errors that I had, so I guess that PR did fix the implementation at least. |
ghost
mentioned this issue
|
@lucacasonato is this issue still present? I cannot find |
|
Yes the issue is still present. The expectations file is now in |
TextEncoder.encodeInto main advantage vs TextEncoder.encode is that it more efficient by lowering pressure on memory allocator. The current implementation was doing a lot of allocations under the hood: - UTF8Encoder is instantiated. - UTF8Encoder allocates arrays as the return value - The string is essentially duplicated by the stringToCodePoints function and then wrapped into a Stream. The implementation also failed some wpt tests (denoland#9006). To fix all of these issues, reimplement UTF-8 encoding in a function that 0 allocations and receives offset states from the caller, while also correctly handling boundaries of the input buffer.
|
@lucacasonato I've managed to fix the ArrayBuffer failures in #10129. The SharedArrayBuffer tests are still failing though, and doesn't seem to be related to the UTF-8 encoding implementation. |
|
It seems the reason SharedArrayBuffer tests are failing is because the constructor is throwing this: |
|
The problem is in test_util/wpt/common/sab.js. It uses Though I assume the correct fix is in ensuring that |
TextEncoder.encodeInto main advantage vs TextEncoder.encode is that it more efficient by lowering pressure on memory allocator. The current implementation was doing a lot of allocations under the hood: - UTF8Encoder is instantiated. - UTF8Encoder allocates arrays as the return value - The string is essentially duplicated by the stringToCodePoints function and then wrapped into a Stream. The implementation also failed some wpt tests (denoland#9006). To fix all of these issues, reimplement UTF-8 encoding in a function that 0 allocations and receives offset states from the caller, while also correctly handling boundaries of the input buffer.
|
@bartlomieju According to @tarruda, the Wasm shared memory was only one part of the problem here, should this be re-opened? |
TextEncoder.encodeInto main advantage vs TextEncoder.encode is that it more efficient by lowering pressure on memory allocator. The current implementation was doing a lot of allocations under the hood: - UTF8Encoder is instantiated. - UTF8Encoder allocates arrays as the return value - The string is essentially duplicated by the stringToCodePoints function and then wrapped into a Stream. The implementation also failed some wpt tests (denoland#9006). To fix all of these issues, reimplement UTF-8 encoding in a function that 0 allocations and receives offset states from the caller, while also correctly handling boundaries of the input buffer.
TextEncoder.encodeInto main advantage vs TextEncoder.encode is that it more efficient by lowering pressure on memory allocator. The current implementation was doing a lot of allocations under the hood: - UTF8Encoder is instantiated. - UTF8Encoder allocates arrays as the return value - The string is essentially duplicated by the stringToCodePoints function and then wrapped into a Stream. The implementation also failed some wpt tests (denoland#9006). To fix all of these issues, reimplement UTF-8 encoding in a function that 0 allocations and receives offset states from the caller, while also correctly handling boundaries of the input buffer.
TextEncoder.encodeInto main advantage vs TextEncoder.encode is that it more efficient by lowering pressure on memory allocator. The current implementation was doing a lot of allocations under the hood: - UTF8Encoder is instantiated. - UTF8Encoder allocates arrays as the return value - The string is essentially duplicated by the stringToCodePoints function and then wrapped into a Stream. The implementation also failed some wpt tests (denoland#9006). To fix all of these issues, reimplement UTF-8 encoding in a function that 0 allocations and receives offset states from the caller, while also correctly handling boundaries of the input buffer.
You can enable the failing WPT tests by adding this snippet to
cli/tests/wpt.jsonc:{ "name": "encodeInto", "expectFail": [ // TODO(lucacasonato): enable once we support MessageChannel "encodeInto() and a detached output buffer" ] },Errors from the failing tests look like this:
The text was updated successfully, but these errors were encountered: