8000 Pre-RFC: Borrowing refactor · Issue #40 · neon-bindings/rfcs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Pre-RFC: Borrowing refactor #40

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.

Sign up for GitHub

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

Jump to bottom
Open
kjvalencik opened this issue Mar 24, 2021 · 1 comment
Open

Pre-RFC: Borrowing refactor #40

kjvalencik opened this issue Mar 24, 2021 · 1 comment

Comments

@kjvalencik
Copy link
Member

Borrow

Problem

The current borrowing API was designed to support borrowing both binary JavaScript data and the internal Rust data of classes. It is intended to prevent aliased borrows.

However, it has multiple soundness holes (JavaScript handles might alias the same object, multiple Lock can be created). It also has an API that includes types and helpers from before NLL that are awkward and unnecessary today.

Lastly, classes were removed in the N-API backend, leaving a generic API that only applies to borrowing binary data.

Proposal

Module

  • Remove neon::borrow
  • Create neon::binary
  • Move binary types (e.g., JsArrayBuffer) to neon::binary
  • Move borrowing into neon::binary

Removal

Buffer is a legacy type from before JavaScript included ArrayBuffer. It is now a sub-class of Uint8Array. Neon does not need any type level distinction for JsBuffer because it can be treated as an array view.

  • Remove JsBuffer
  • Only reference to buffer is a constructor on ArrayBuffer for creating a Buffer. The return type is indistinguishable from a Uint8Array in Neon.

Addition

Neon does not currently understand views. We will need to add this. We most likely do not need to know the view type and can have a simple JsArrayView. If we want to include the type, it could be done with a generic phantom data element. JsArrayView<u8>.

Borrowing

In an initial implementation, we will only support borrowing ArrayBuffer. This way we can avoid complicated set math for determining if any of our borrows overlap. We can simply keep a HashSet of starting pointers.

Lock

The Lock type will be removed from the public API and moved to Context.

Views

Borrowing views will mostly be a convenience API. We will still borrow the entire ArrayBuffer. This will behave very similar to how borrowing a &[u8] from a Vec<u8> borrows the entire Vec.

This is necessary to remove complexity from users. If we don't provide this API, users cannot borrow buffers without first either copying data to an ArrayBuffer or passing the underlying buffer along with a Range.
@dherman
Copy link
Contributor
dherman commented Mar 27, 2021

Great proposal, thanks for writing it up!

A couple possible tweaks:

  • Maybe JsArrayView should be called JsArrayBufferView to match the standard name?
  • I'd suggest the buffer constructor should be on the view type instead of JsArrayBuffer, since Buffer <: Uint8Array <: ArrayBufferView (i.e. a buffer is-a ArrayBufferView and has-a ArrayBuffer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@dherman @kjvalencik
0