Skip to content

Commit

Permalink
perf(ops): Remove unnecessary fast call fallback options usage (denol…
Browse files Browse the repository at this point in the history
…and#17585)

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.
  • Loading branch information
aapoalas authored Jan 29, 2023
1 parent 2c78550 commit 04ba709
Show file tree
Hide file tree
Showing 8 changed files with 23 additions and 55 deletions.
8 changes: 3 additions & 5 deletions ext/flash/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -373,11 +373,9 @@ unsafe fn op_flash_respond_fast(
let ctx = &mut *(ptr as *mut ServerContext);

let response = &*response;
if let Some(response) = response.get_storage_if_aligned() {
flash_respond(ctx, token, shutdown, response)
} else {
todo!();
}
// Uint8Array is always byte-aligned.
let response = response.get_storage_if_aligned().unwrap_unchecked();
flash_respond(ctx, token, shutdown, response)
}

macro_rules! get_request {
Expand Down
29 changes: 12 additions & 17 deletions ops/optimizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ impl Transform {
parse_quote! { *const #core::v8::fast_api::FastApiTypedArray<u32> };

q!(Vars { var: &ident }, {
// V8 guarantees that ArrayBuffers are always 4-byte aligned
// (seems to be always 8-byte aligned on 64-bit machines)
// but Deno FFI makes it possible to create ArrayBuffers at any
// alignment. Thus this check is needed.
let var = match unsafe { &*var }.get_storage_if_aligned() {
Some(v) => v,
None => {
Expand All @@ -141,17 +145,14 @@ impl Transform {
parse_quote! { *const #core::v8::fast_api::FastApiTypedArray<u8> };

q!(Vars { var: &ident }, {
let var = match unsafe { &*var }.get_storage_if_aligned() {
Some(v) => v,
None => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
// SAFETY: U8 slice is always byte-aligned.
let var =
unsafe { (&*var).get_storage_if_aligned().unwrap_unchecked() };
})
}
TransformKind::WasmMemory => {
// Note: `ty` is correctly set to __opts by the fast call tier.
// U8 slice is always byte-aligned.
q!(Vars { var: &ident, core }, {
let var = unsafe {
&*(__opts.wasm_memory
Expand All @@ -166,13 +167,10 @@ impl Transform {
parse_quote! { *const #core::v8::fast_api::FastApiTypedArray<u8> };

q!(Vars { var: &ident }, {
let var = match unsafe { &*var }.get_storage_if_aligned() {
Some(v) => v.as_ptr(),
None => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
// SAFETY: U8 slice is always byte-aligned.
let var =
unsafe { (&*var).get_storage_if_aligned().unwrap_unchecked() }
.as_ptr();
})
}
}
Expand Down Expand Up @@ -473,7 +471,6 @@ impl Optimizer {
match segment {
// Is `T` a u8?
PathSegment { ident, .. } if ident == "u8" => {
self.needs_fast_callback_option = true;
assert!(self
.transforms
.insert(index, Transform::wasm_memory(index))
Expand Down Expand Up @@ -608,7 +605,6 @@ impl Optimizer {
match segment {
// Is `T` a u8?
PathSegment { ident, .. } if ident == "u8" => {
self.needs_fast_callback_option = true;
self.fast_parameters.push(FastValue::Uint8Array);
assert!(self
.transforms
Expand Down Expand Up @@ -645,7 +641,6 @@ impl Optimizer {
match segment {
// Is `T` a u8?
PathSegment { ident, .. } if ident == "u8" => {
self.needs_fast_callback_option = true;
self.fast_parameters.push(FastValue::Uint8Array);
assert!(self
.transforms
Expand Down
2 changes: 1 addition & 1 deletion ops/optimizer_tests/op_state_with_transforms.expected
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: false
has_ref_opstate: true
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: true
needs_fast_callback_option: false
fast_result: Some(Void)
fast_parameters: [V8Value, Uint8Array]
transforms: {1: Transform { kind: SliceU8(true), index: 1 }}
Expand Down
8 changes: 1 addition & 7 deletions ops/optimizer_tests/op_state_with_transforms.out
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,7 @@ where
as *const _ops::OpCtx)
};
let state = &mut ::std::cell::RefCell::borrow_mut(&__ctx.state);
let buf = match unsafe { &*buf }.get_storage_if_aligned() {
Some(v) => v,
None => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
let buf = unsafe { (&*buf).get_storage_if_aligned().unwrap_unchecked() };
let result = op_now::call::<TP>(state, buf);
result
}
8 changes: 1 addition & 7 deletions ops/optimizer_tests/raw_ptr.out
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,7 @@ where
as *const _ops::OpCtx)
};
let state = &mut ::std::cell::RefCell::borrow_mut(&__ctx.state);
let buf = match unsafe { &*buf }.get_storage_if_aligned() {
Some(v) => v.as_ptr(),
None => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
let buf = unsafe { (&*buf).get_storage_if_aligned().unwrap_unchecked() }.as_ptr();
let out = match unsafe { &*out }.get_storage_if_aligned() {
Some(v) => v,
None => {
Expand Down
2 changes: 1 addition & 1 deletion ops/optimizer_tests/uint8array.expected
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: false
has_ref_opstate: false
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: true
needs_fast_callback_option: false
fast_result: Some(Bool)
fast_parameters: [V8Value, Uint8Array, Uint8Array]
transforms: {0: Transform { kind: SliceU8(false), index: 0 }, 1: Transform { kind: SliceU8(true), index: 1 }}
Expand Down
19 changes: 3 additions & 16 deletions ops/optimizer_tests/uint8array.out
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ impl<'scope> deno_core::v8::fast_api::FastFunction for op_import_spki_x25519_fas
fn args(&self) -> &'static [deno_core::v8::fast_api::Type] {
use deno_core::v8::fast_api::Type::*;
use deno_core::v8::fast_api::CType;
&[V8Value, TypedArray(CType::Uint8), TypedArray(CType::Uint8), CallbackOptions]
&[V8Value, TypedArray(CType::Uint8), TypedArray(CType::Uint8)]
}
fn return_type(&self) -> deno_core::v8::fast_api::CType {
deno_core::v8::fast_api::CType::Bool
Expand All @@ -168,24 +168,11 @@ fn op_import_spki_x25519_fast_fn<'scope>(
_: deno_core::v8::Local<deno_core::v8::Object>,
key_data: *const deno_core::v8::fast_api::FastApiTypedArray<u8>,
out: *const deno_core::v8::fast_api::FastApiTypedArray<u8>,
fast_api_callback_options: *mut deno_core::v8::fast_api::FastApiCallbackOptions,
) -> bool {
use deno_core::v8;
use deno_core::_ops;
let key_data = match unsafe { &*key_data }.get_storage_if_aligned() {
Some(v) => v,
None => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
let out = match unsafe { &*out }.get_storage_if_aligned() {
Some(v) => v,
None => {
unsafe { &mut *fast_api_callback_options }.fallback = true;
return Default::default();
}
};
let key_data = unsafe { (&*key_data).get_storage_if_aligned().unwrap_unchecked() };
let out = unsafe { (&*out).get_storage_if_aligned().unwrap_unchecked() };
let result = op_import_spki_x25519::call(key_data, out);
result
}
2 changes: 1 addition & 1 deletion ops/optimizer_tests/wasm_op.expected
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ returns_result: false
has_ref_opstate: false
has_rc_opstate: false
has_fast_callback_option: false
needs_fast_callback_option: true
needs_fast_callback_option: false
fast_result: Some(Void)
fast_parameters: [V8Value]
transforms: {0: Transform { kind: WasmMemory, index: 0 }}
Expand Down

0 comments on commit 04ba709

Please sign in to comment.