RFC: VM 2.0 #14

dherman · 2017-12-20T06:40:00Z

Replaces four abstractions of the existing Neon API (Scope, Arguments, FunctionCall, and the vm module) with one Vm trait, representing a (context-sensitive) view of the JavaScript virtual machine.

Rendered

WIP Implementation

sgrif · 2017-12-20T22:14:02Z

text/vm-2.0.md

+For example, a simple Neon function that takes a single `ArrayBuffer` object and sets its first byte to 0 would be implemented by calling `vm.lock()` and then `lock.inspect()` with the resulting lock object:
+
+```rust
+fn zero_first_byte(mut vm: impl Vm) -> JsResult<Handle<JsNumber>> {


Is there a reason that this needs to rely on unstable features? Could this just be fn zero_first_byte<T: Vm>(mut vm: T)?

Oh I was just using the shorthand because it makes me happy :P There's no inherent reliance on it. I should eliminate that to avoid confusion.

matklad · 2017-12-21T16:26:38Z

text/vm-2.0.md

+
+```rust
+impl Lock {
+    fn inspect<T: Inspect>(&self, value: Handle<T>) -> &mut T::Internals;


This does not look quite right: the Handle is copy, so we can get two aliased &mut T::Internals?

Yes, good point. The way the current locking mechanism works is by dynamically tracking what ranges in memory are currently being inspected. We can store that state inside the Lock object itself, and either make inspect an &mut self method or store that state in a cell.

The way the current locking mechanism works is by dynamically tracking what ranges in memory are currently being inspected.

Yeah, so I would expect the return type to be RefMut<T::Internals>, like RefCell works.

Really interesting point. If we're going all the way to consistency with the stdlib, it makes me wonder if that suggests refactoring the API so that Inspect gets a name more like RefValue or something like that, with:

trait RefValue<T> { fn borrow(&self, lock: &Lock) -> Ref<T>; fn borrow_mut(&mut self, lock: &Lock) -> RefMut<T>; }

So that you would write examples like zero_first_byte from the RFC like so:

fn zero_first_byte(mut vm: impl Vm) -> JsResult<Handle<JsNumber>> { let array_buffer = vm.argument(0).unwrap().check::<JsArrayBuffer>()?; let lock = vm.lock(); let mut contents = array_buffer.borrow_mut(&lock); let mut buf = contents.as_u8_slice(); buf[0] = 0; Ok(()) }

It's appealing to use the nomenclature of borrowing, although since I haven't worked with ref cells in practice, I don't know what kind of learnability hurdles they might introduce. Maybe it's no worse than the learnability of the Inspect API though? I really like reusing the intuitions of borrowing for understanding what it means to temporarily inspect the contents of a JS value.

If we're going all the way to consistency with the stdlib,

My point was not about consistency with stdlib (though that'd be nice!). I think that if you do run-time tracking, you'll have to use either closure-base APIs, or return some sort of lock-object, like RefMut. Returning plain &mut T I think will be unsond.

Hm, I don't see how to make it work with the actual Ref/RefMut types of std::cell, since those have no constructors and the only way to get one is to call borrow{_mut}() on a RefCell. I could do something analogous with a custom Neon Ref and RefMut type though.

I suppose the soundness issue is needing to track the duration of the borrows? And having a wrapper type allows us to register the borrow on allocation of the Ref{Mut} and unregister it on drop of the Ref{Mut}?

Yeah, I totally didn't mean that we should use std::cell::RefMut, that's impossible. But we quite probably will have to roll our own neon::RefMut.

Got it, thanks! This makes sense to me, and I really like the way the API feels.

I suppose the soundness issue is needing to track the duration of the borrows? And having a wrapper type allows us to register the borrow on allocation of the Ref{Mut} and unregister it on drop of the Ref{Mut}?

Exactly, std::RefCell is basically a single-threaded std::Mutex, and std::RefMut is std::LockGuard.

tptee

I'm really pumped to see this happen 😄 The API ergonomics are a huge leap forward and I think they'll lower the barrier of entry a good bit.

I never understood nest/chain, so seeing the docs and renames to execute_scope and compute_scope are particularly helpful. Love the shorthands on CallContext as well!

tptee · 2018-07-03T04:23:49Z

text/vm-2.0.md

+    fn argument_opt(&mut self, i: i32) -> Option<Handle<'a, JsValue>>;
+    fn argument<V: Value>(&mut self, i: i32) -> JsResult<'a, V>;
+    fn this(&mut self) -> Handle<'a, T>;
+    fn callee(&mut self) -> Handle<'a, JsFunction>;


Are we leaving this out of VM 2.0 or removing it later before the 0.2 release?

Good catch! I'll remove it from the RFC.

dherman · 2018-07-05T18:00:52Z

📢 This RFC is going into final comment period! We will plan to merge it within a week if there aren't significant new issues that arise in the next week. 📢

…e the `impl Trait` syntax explicitly since it's not yet part of the wider Rust vocabulary

…obj.borrow_mut(&lock)` and define `Ref` and `RefMut` types to track the duration of the dynamic borrow

- add `FunctionContext`, `MethodContext` shorthands

…r()`

sgrif reviewed Dec 20, 2017

View reviewed changes

matklad reviewed Dec 21, 2017

View reviewed changes

tptee approved these changes Jul 3, 2018

View reviewed changes

dherman added the final comment period last opportunity to discuss the proposed conclusion label Jul 5, 2018

dherman added 15 commits July 12, 2018 07:34

RFC: VM 2.0

9e1c301

tie in to future Classes 2.0 RFC

7dc3781

formatting edits

0953c8d

introduce the VM parameter more gradually in the guide; also introduc…

ad490ed

…e the `impl Trait` syntax explicitly since it's not yet part of the wider Rust vocabulary

API improvement thanks to @matklad: refactor lock.inspect(obj) to `…

74e9b0e

…obj.borrow_mut(&lock)` and define `Ref` and `RefMut` types to track the duration of the dynamic borrow

lots of updates based on implementation experience

79fbf51

- delete "indexing into CallContext" section

508074f

- add `FunctionContext`, `MethodContext` shorthands

editorial improvements

6ac7c15

fixed signature of compute_scoped

87880d8

another detailed correctness question for open issues

ce88c73

one more open question (bikesheds woo!)

10490a1

CallContext::{this, callee} are infallible

e450a57

add Context::borrow(), Context::borrow_mut(), and `Context::buffe…

ede6295

…r()`

Drop callee from the RFC.

c900f84

Rename file.

2a7bdc2

dherman force-pushed the vm-two-point-oh branch from 1f2c0e2 to 2a7bdc2 Compare July 12, 2018 14:35

dherman merged commit e67ff40 into neon-bindings:master Jul 25, 2018

dherman deleted the vm-two-point-oh branch July 25, 2018 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: VM 2.0 #14

RFC: VM 2.0 #14

dherman commented Dec 20, 2017 •

edited

Loading

sgrif Dec 20, 2017

dherman Dec 21, 2017

matklad Dec 21, 2017

dherman Dec 27, 2017

matklad Dec 27, 2017

dherman Dec 27, 2017

matklad Dec 27, 2017

dherman Dec 27, 2017

dherman Dec 27, 2017

matklad Dec 27, 2017

dherman Dec 27, 2017

matklad Dec 27, 2017

tptee left a comment

tptee Jul 3, 2018

dherman Jul 5, 2018

dherman commented Jul 5, 2018

RFC: VM 2.0 #14

RFC: VM 2.0 #14

Conversation

dherman commented Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tptee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dherman commented Jul 5, 2018

dherman commented Dec 20, 2017 •

edited

Loading