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

perf(ops): Remove unnecessary fast call fallback options usage #17585

Merged

Conversation

aapoalas
Copy link
Collaborator

Currently fast ops will always check for the alignment of a TypedArray when getting a slice out of them. A match is then done to ensure that some slice was received and if not a fallback will be requested.

For Uint8Arrays (and WasmMemory which is equivalent to a Uint8Array) the alignment will always be okay. Rust probably optimises this away for the most part (since the Uint8Array check is x % 1 != 0), but what it cannot optimise away is the fast ops path's request for fallback options parameter.

The extra parameter's cost is likely negligible but V8 will need to check if a fallback was requested and prepare the fallback call just in case it was. In the future the lack of a fallback may also enable V8 to much better optimise the result handling.

For V8 created buffers, it seems like all buffers are actually always guaranteed to be properly aligned: All buffers seem to always be created 8-byte aligned, and creating a 32 bit array or 64 bit array with a non-aligned offset from an ArrayBuffer is not allowed. Unfortunately, Deno FFI cannot give the same guarantees, and it is actually possible for eg. 32 bit arrays to be created unaligned using it. These arrays work fine (at least on Linux) so it seems like this is not illegal, it just means that we cannot remove the alignment checking for 32 bit arrays.

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice!

I attempted this too in #16918

@littledivy littledivy merged commit 04ba709 into denoland:main Jan 29, 2023
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 this pull request may close these issues.

2 participants