From 60c008d23b2bdad333711b43148a5053e83a62cc Mon Sep 17 00:00:00 2001 From: F001 Date: Wed, 5 Dec 2018 12:36:10 +0800 Subject: [PATCH] Isolate::from_raw_ptr and other cleanups. `Isolate::from_void_ptr` is renamed to `from_raw_ptr`, to keep consistency with std libs. It is changed to `unsafe` function, because it can't guarantee that the input is valid. This guarantee should be provided by the caller. Its return type is changed to `&Isolate`, because `&mut Isolate` type requires that no other aliases co-exist in this period of time, this does not seem true. So I changed most of the methods to accept shared reference `&Isolate`. It is easier to reason about the correctness of `unsafe` blocks. As long as these shared references are in the same thread, these `unsafe` codes are probably correct. --- src/isolate.rs | 94 ++++++++++++++++++++++++++++++-------------------- src/libdeno.rs | 17 +++++++-- src/main.rs | 2 +- src/ops.rs | 89 +++++++++++++++++++++++------------------------ 4 files changed, 117 insertions(+), 85 deletions(-) diff --git a/src/isolate.rs b/src/isolate.rs index 3379eb80d0..6b2814ec4c 100644 --- a/src/isolate.rs +++ b/src/isolate.rs @@ -14,6 +14,7 @@ use permissions::DenoPermissions; use futures::Future; use libc::c_void; use std; +use std::cell::Cell; use std::env; use std::ffi::CStr; use std::ffi::CString; @@ -38,19 +39,17 @@ pub type Buf = Box<[u8]>; pub type Op = Future + Send; // Returns (is_sync, op) -pub type Dispatch = fn( - isolate: &mut Isolate, - buf: libdeno::deno_buf, - data_buf: libdeno::deno_buf, -) -> (bool, Box); +pub type Dispatch = + fn(isolate: &Isolate, buf: libdeno::deno_buf, data_buf: libdeno::deno_buf) + -> (bool, Box); pub struct Isolate { libdeno_isolate: *const libdeno::isolate, dispatch: Dispatch, rx: mpsc::Receiver<(i32, Buf)>, tx: mpsc::Sender<(i32, Buf)>, - ntasks: i32, - pub timeout_due: Option, + ntasks: Cell, + timeout_due: Cell>, pub state: Arc, } @@ -79,18 +78,22 @@ impl IsolateState { } } + #[inline] pub fn check_write(&self, filename: &str) -> DenoResult<()> { self.permissions.check_write(filename) } + #[inline] pub fn check_env(&self) -> DenoResult<()> { self.permissions.check_env() } + #[inline] pub fn check_net(&self, filename: &str) -> DenoResult<()> { self.permissions.check_net(filename) } + #[inline] pub fn check_run(&self) -> DenoResult<()> { self.permissions.check_run() } @@ -154,23 +157,35 @@ impl Isolate { dispatch, rx, tx, - ntasks: 0, - timeout_due: None, + ntasks: Cell::new(0), + timeout_due: Cell::new(None), state, } } - pub fn as_void_ptr(&mut self) -> *mut c_void { - self as *mut _ as *mut c_void + #[inline] + pub fn as_raw_ptr(&self) -> *const c_void { + self as *const _ as *const c_void } - pub fn from_void_ptr<'a>(ptr: *mut c_void) -> &'a mut Self { - let ptr = ptr as *mut _; - unsafe { &mut *ptr } + #[inline] + pub unsafe fn from_raw_ptr<'a>(ptr: *const c_void) -> &'a Self { + let ptr = ptr as *const _; + &*ptr + } + + #[inline] + pub fn get_timeout_due(&self) -> Option { + self.timeout_due.clone().into_inner() + } + + #[inline] + pub fn set_timeout_due(&self, inst: Option) { + self.timeout_due.set(inst); } pub fn execute( - &mut self, + &self, js_filename: &str, js_source: &str, ) -> Result<(), DenoException> { @@ -179,7 +194,7 @@ impl Isolate { let r = unsafe { libdeno::deno_execute( self.libdeno_isolate, - self.as_void_ptr(), + self.as_raw_ptr(), filename.as_ptr(), source.as_ptr(), ) @@ -192,21 +207,21 @@ impl Isolate { Ok(()) } - pub fn respond(&mut self, req_id: i32, buf: Buf) { + pub fn respond(&self, req_id: i32, buf: Buf) { self.state.metrics_op_completed(buf.len()); // deno_respond will memcpy the buf into V8's heap, // so borrowing a reference here is sufficient. unsafe { libdeno::deno_respond( self.libdeno_isolate, - self.as_void_ptr(), + self.as_raw_ptr(), req_id, buf.as_ref().into(), ) } } - fn complete_op(&mut self, req_id: i32, buf: Buf) { + fn complete_op(&self, req_id: i32, buf: Buf) { // Receiving a message on rx exactly corresponds to an async task // completing. self.ntasks_decrement(); @@ -214,12 +229,12 @@ impl Isolate { self.respond(req_id, buf); } - fn timeout(&mut self) { + fn timeout(&self) { let dummy_buf = libdeno::deno_buf::empty(); unsafe { libdeno::deno_respond( self.libdeno_isolate, - self.as_void_ptr(), + self.as_raw_ptr(), -1, dummy_buf, ) @@ -234,10 +249,10 @@ impl Isolate { // TODO Use Park abstraction? Note at time of writing Tokio default runtime // does not have new_with_park(). - pub fn event_loop(&mut self) { + pub fn event_loop(&self) { // Main thread event loop. while !self.is_idle() { - match recv_deadline(&self.rx, self.timeout_due) { + match recv_deadline(&self.rx, self.get_timeout_due()) { Ok((req_id, buf)) => self.complete_op(req_id, buf), Err(mpsc::RecvTimeoutError::Timeout) => self.timeout(), Err(e) => panic!("recv_deadline() failed: {:?}", e), @@ -248,18 +263,21 @@ impl Isolate { self.check_promise_errors(); } - fn ntasks_increment(&mut self) { - assert!(self.ntasks >= 0); - self.ntasks += 1; + #[inline] + fn ntasks_increment(&self) { + assert!(self.ntasks.get() >= 0); + self.ntasks.set(self.ntasks.get() + 1); } - fn ntasks_decrement(&mut self) { - self.ntasks -= 1; - assert!(self.ntasks >= 0); + #[inline] + fn ntasks_decrement(&self) { + self.ntasks.set(self.ntasks.get() - 1); + assert!(self.ntasks.get() >= 0); } + #[inline] fn is_idle(&self) -> bool { - self.ntasks == 0 && self.timeout_due.is_none() + self.ntasks.get() == 0 && self.get_timeout_due().is_none() } } @@ -280,7 +298,9 @@ extern "C" fn pre_dispatch( let bytes_sent_control = control_buf.len(); let bytes_sent_data = data_buf.len(); - let isolate = Isolate::from_void_ptr(user_data); + // We should ensure that there is no other `&mut Isolate` exists. + // And also, it should be in the same thread with other `&Isolate`s. + let isolate = unsafe { Isolate::from_raw_ptr(user_data) }; let dispatch = isolate.dispatch; let (is_sync, op) = dispatch(isolate, control_buf, data_buf); @@ -353,7 +373,7 @@ mod tests { let state = Arc::new(IsolateState::new(flags, rest_argv)); let snapshot = libdeno::deno_buf::empty(); - let mut isolate = Isolate::new(snapshot, state, dispatch_sync); + let isolate = Isolate::new(snapshot, state, dispatch_sync); tokio_util::init(|| { isolate .execute( @@ -374,7 +394,7 @@ mod tests { } fn dispatch_sync( - _isolate: &mut Isolate, + _isolate: &Isolate, control: libdeno::deno_buf, data: libdeno::deno_buf, ) -> (bool, Box) { @@ -395,7 +415,7 @@ mod tests { let (flags, rest_argv, _) = flags::set_flags(argv).unwrap(); let state = Arc::new(IsolateState::new(flags, rest_argv)); let snapshot = libdeno::deno_buf::empty(); - let mut isolate = Isolate::new(snapshot, state, metrics_dispatch_sync); + let isolate = Isolate::new(snapshot, state, metrics_dispatch_sync); tokio_util::init(|| { // Verify that metrics have been properly initialized. { @@ -432,7 +452,7 @@ mod tests { let (flags, rest_argv, _) = flags::set_flags(argv).unwrap(); let state = Arc::new(IsolateState::new(flags, rest_argv)); let snapshot = libdeno::deno_buf::empty(); - let mut isolate = Isolate::new(snapshot, state, metrics_dispatch_async); + let isolate = Isolate::new(snapshot, state, metrics_dispatch_async); tokio_util::init(|| { // Verify that metrics have been properly initialized. { @@ -481,7 +501,7 @@ mod tests { } fn metrics_dispatch_sync( - _isolate: &mut Isolate, + _isolate: &Isolate, _control: libdeno::deno_buf, _data: libdeno::deno_buf, ) -> (bool, Box) { @@ -492,7 +512,7 @@ mod tests { } fn metrics_dispatch_async( - _isolate: &mut Isolate, + _isolate: &Isolate, _control: libdeno::deno_buf, _data: libdeno::deno_buf, ) -> (bool, Box) { diff --git a/src/libdeno.rs b/src/libdeno.rs index 3e41c7d605..cbc074f1be 100644 --- a/src/libdeno.rs +++ b/src/libdeno.rs @@ -31,6 +31,7 @@ pub struct deno_buf { unsafe impl Send for deno_buf {} impl deno_buf { + #[inline] pub fn empty() -> Self { Self { alloc_ptr: null(), @@ -40,6 +41,7 @@ impl deno_buf { } } + #[inline] pub unsafe fn from_raw_parts(ptr: *const u8, len: usize) -> Self { Self { alloc_ptr: null(), @@ -52,6 +54,7 @@ impl deno_buf { /// Converts Rust &Buf to libdeno `deno_buf`. impl<'a> From<&'a [u8]> for deno_buf { + #[inline] fn from(x: &'a [u8]) -> Self { Self { alloc_ptr: null(), @@ -64,27 +67,37 @@ impl<'a> From<&'a [u8]> for deno_buf { impl Deref for deno_buf { type Target = [u8]; + #[inline] fn deref(&self) -> &[u8] { unsafe { std::slice::from_raw_parts(self.data_ptr, self.data_len) } } } impl DerefMut for deno_buf { + #[inline] fn deref_mut(&mut self) -> &mut [u8] { unsafe { + if self.alloc_ptr.is_null() { + panic!("Can't modify the buf"); + } std::slice::from_raw_parts_mut(self.data_ptr as *mut u8, self.data_len) } } } impl AsRef<[u8]> for deno_buf { + #[inline] fn as_ref(&self) -> &[u8] { &*self } } impl AsMut<[u8]> for deno_buf { + #[inline] fn as_mut(&mut self) -> &mut [u8] { + if self.alloc_ptr.is_null() { + panic!("Can't modify the buf"); + } &mut *self } } @@ -112,13 +125,13 @@ extern "C" { pub fn deno_check_promise_errors(i: *const isolate); pub fn deno_respond( i: *const isolate, - user_data: *mut c_void, + user_data: *const c_void, req_id: i32, buf: deno_buf, ); pub fn deno_execute( i: *const isolate, - user_data: *mut c_void, + user_data: *const c_void, js_filename: *const c_char, js_source: *const c_char, ) -> c_int; diff --git a/src/main.rs b/src/main.rs index 1224d2af20..f2e241ab16 100644 --- a/src/main.rs +++ b/src/main.rs @@ -98,7 +98,7 @@ fn main() { let state = Arc::new(isolate::IsolateState::new(flags, rest_argv)); let snapshot = snapshot::deno_snapshot(); - let mut isolate = isolate::Isolate::new(snapshot, state, ops::dispatch); + let isolate = isolate::Isolate::new(snapshot, state, ops::dispatch); tokio_util::init(|| { isolate .execute("deno_main.js", "denoMain();") diff --git a/src/ops.rs b/src/ops.rs index 908de1e230..7c4fc5119f 100644 --- a/src/ops.rs +++ b/src/ops.rs @@ -35,7 +35,6 @@ use std::path::Path; use std::path::PathBuf; use std::process::Command; use std::str::FromStr; -use std::sync::Arc; use std::time::UNIX_EPOCH; use std::time::{Duration, Instant}; use tokio; @@ -49,10 +48,10 @@ type OpResult = DenoResult; // TODO Ideally we wouldn't have to box the Op being returned. // The box is just to make it easier to get a prototype refactor working. type OpCreator = - fn(state: &Arc, base: &msg::Base, data: libdeno::deno_buf) + fn(state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf) -> Box; -// Hopefully Rust optimizes this away. +#[inline] fn empty_buf() -> Buf { Box::new([]) } @@ -62,7 +61,7 @@ fn empty_buf() -> Buf { /// control corresponds to the first argument of libdeno.send(). /// data corresponds to the second argument of libdeno.send(). pub fn dispatch( - isolate: &mut Isolate, + isolate: &Isolate, control: libdeno::deno_buf, data: libdeno::deno_buf, ) -> (bool, Box) { @@ -121,7 +120,7 @@ pub fn dispatch( msg::enum_name_any(inner_type) )), }; - op_creator(&isolate.state.clone(), &base, data) + op_creator(&isolate.state, &base, data) }; let boxed_op = Box::new( @@ -169,7 +168,7 @@ pub fn dispatch( } fn op_exit( - _config: &Arc, + _config: &IsolateState, base: &msg::Base, _data: libdeno::deno_buf, ) -> Box { @@ -178,7 +177,7 @@ fn op_exit( } fn op_start( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -234,8 +233,7 @@ fn serialize_response( msg::finish_base_buffer(builder, base); let data = builder.finished_data(); // println!("serialize_response {:x?}", data); - let vec = data.to_vec(); - vec.into_boxed_slice() + data.into() } fn ok_future(buf: Buf) -> Box { @@ -249,7 +247,7 @@ fn odd_future(err: DenoError) -> Box { // https://github.com/denoland/deno/blob/golang/os.go#L100-L154 fn op_code_fetch( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -292,7 +290,7 @@ fn op_code_fetch( // https://github.com/denoland/deno/blob/golang/os.go#L156-L169 fn op_code_cache( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -311,7 +309,7 @@ fn op_code_cache( } fn op_chdir( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -325,7 +323,7 @@ fn op_chdir( } fn op_set_timeout( - isolate: &mut Isolate, + isolate: &Isolate, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -333,16 +331,17 @@ fn op_set_timeout( let inner = base.inner_as_set_timeout().unwrap(); // FIXME why is timeout a double if it's cast immediately to i64/u64?? let val = inner.timeout() as i64; - isolate.timeout_due = if val >= 0 { + let timeout_due = if val >= 0 { Some(Instant::now() + Duration::from_millis(val as u64)) } else { None }; + isolate.set_timeout_due(timeout_due); ok_future(empty_buf()) } fn op_set_env( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -358,7 +357,7 @@ fn op_set_env( } fn op_env( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -390,7 +389,7 @@ fn op_env( } fn op_fetch( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -479,7 +478,7 @@ macro_rules! blocking { } fn op_make_temp_dir( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -528,7 +527,7 @@ fn op_make_temp_dir( } fn op_mkdir( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -548,7 +547,7 @@ fn op_mkdir( } fn op_chmod( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -581,7 +580,7 @@ fn op_chmod( } fn op_open( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -612,7 +611,7 @@ fn op_open( } fn op_close( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -629,7 +628,7 @@ fn op_close( } fn op_shutdown( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -655,7 +654,7 @@ fn op_shutdown( } fn op_read( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -693,7 +692,7 @@ fn op_read( } fn op_write( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -730,7 +729,7 @@ fn op_write( } fn op_remove( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -760,7 +759,7 @@ fn op_remove( // Prototype https://github.com/denoland/deno/blob/golang/os.go#L171-L184 fn op_read_file( - _config: &Arc, + _config: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -794,7 +793,7 @@ fn op_read_file( } fn op_copy_file( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -846,7 +845,7 @@ fn get_mode(_perm: &fs::Permissions) -> u32 { } fn op_cwd( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -872,7 +871,7 @@ fn op_cwd( } fn op_stat( - _config: &Arc, + _config: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -919,7 +918,7 @@ fn op_stat( } fn op_read_dir( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -975,7 +974,7 @@ fn op_read_dir( } fn op_write_file( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -995,7 +994,7 @@ fn op_write_file( } fn op_rename( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1015,7 +1014,7 @@ fn op_rename( } fn op_symlink( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1044,7 +1043,7 @@ fn op_symlink( } fn op_read_link( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1077,7 +1076,7 @@ fn op_read_link( } fn op_repl_start( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1108,7 +1107,7 @@ fn op_repl_start( } fn op_repl_readline( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1146,7 +1145,7 @@ fn op_repl_readline( } fn op_truncate( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1169,7 +1168,7 @@ fn op_truncate( } fn op_listen( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1235,7 +1234,7 @@ fn new_conn(cmd_id: u32, tcp_stream: TcpStream) -> OpResult { } fn op_accept( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1261,7 +1260,7 @@ fn op_accept( } fn op_dial( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1285,7 +1284,7 @@ fn op_dial( } fn op_metrics( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1309,7 +1308,7 @@ fn op_metrics( } fn op_resources( - _state: &Arc, + _state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1361,7 +1360,7 @@ fn subprocess_stdio_map(v: msg::ProcessStdio) -> std::process::Stdio { } fn op_run( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box { @@ -1429,7 +1428,7 @@ fn op_run( } fn op_run_status( - state: &Arc, + state: &IsolateState, base: &msg::Base, data: libdeno::deno_buf, ) -> Box {