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

feat(ext/ffi): Make op_ffi_ptr_of fast #16297

Merged
merged 4 commits into from
Oct 20, 2022

Conversation

aapoalas
Copy link
Collaborator

Makes op_ffi_ptr_of fast. One of the tests changed from printing false to true as the fast &[u8] slice path creates the slice with a null pointer. Thus the op_ffi_ptr_of will now return a null pointer value whereas previously it returned a dangling pointer value.

ext/ffi/lib.rs Outdated
Comment on lines 2079 to 2083
let outptr = out.as_ptr();
let outptr_integer = outptr as usize;
let length = out.len();
if length < (std::mem::size_of::<usize>() / std::mem::size_of::<u32>())
|| (outptr_integer % std::mem::size_of::<usize>()) != 0
Copy link
Member

Choose a reason for hiding this comment

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

#[op(fast)]
fn op_ffi_ptr_of<FP>(
  state: &mut deno_core::OpState,
  buf: &mut [u8],
  out: &mut [u32],
) -> Result<(), AnyError>
where
  FP: FfiPermissions + 'static,
{
  check_unstable(state, "Deno.UnsafePointer#of");
  let permissions = state.borrow_mut::<FP>();
  permissions.check(None)?;

  let ptr = buf.as_ptr() as u64;

  out[0] = ptr as u32;
  out[1] = (ptr >> 32) as u32;
  Ok(())
}

out[0]/out[1] will panic out of bounds so the length check is also not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it preferable that a panic occurs or should it rather be a throw?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, I would presume that it'd be faster to write in a single "operation" rather than do the shifting etc. I was actually thinking of perhaps making the checks debug_asserts.

Copy link
Member

Choose a reason for hiding this comment

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

yes panic is better when its not possible to invoke this from a public API. It forces users to report bugs

Copy link
Collaborator Author

@aapoalas aapoalas Oct 19, 2022

Choose a reason for hiding this comment

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

I changed these to asserts and went with a write-to-pointer-of-usize handling instead.

@littledivy littledivy merged commit e2be70b into denoland:main Oct 20, 2022
@aapoalas aapoalas deleted the feat/ext-ffi/fast-ptr-of branch December 22, 2022 18:46
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.

None yet

2 participants