Skip to content

Commit

Permalink
Refactor dispatch handling (#2452)
Browse files Browse the repository at this point in the history
Promise id is now created in core and passed back to JS.
  • Loading branch information
afinch7 authored and ry committed Jun 14, 2019
1 parent fdd2eb5 commit dc60fe9
Show file tree
Hide file tree
Showing 18 changed files with 667 additions and 709 deletions.
45 changes: 15 additions & 30 deletions cli/dispatch_minimal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//! message or a "minimal" message.
use crate::state::ThreadSafeState;
use deno::Buf;
use deno::CoreOp;
use deno::Op;
use deno::PinnedBuf;
use futures::Future;
Expand All @@ -17,23 +18,16 @@ const OP_WRITE: i32 = 2;
#[derive(Copy, Clone, Debug, PartialEq)]
// This corresponds to RecordMinimal on the TS side.
pub struct Record {
pub promise_id: i32,
pub op_id: i32,
pub arg: i32,
pub result: i32,
}

impl Into<Buf> for Record {
fn into(self) -> Buf {
let vec = vec![
DISPATCH_MINIMAL_TOKEN,
self.promise_id,
self.op_id,
self.arg,
self.result,
];
let vec = vec![DISPATCH_MINIMAL_TOKEN, self.op_id, self.arg, self.result];
let buf32 = vec.into_boxed_slice();
let ptr = Box::into_raw(buf32) as *mut [u8; 5 * 4];
let ptr = Box::into_raw(buf32) as *mut [u8; 4 * 4];
unsafe { Box::from_raw(ptr) }
}
}
Expand All @@ -45,36 +39,32 @@ pub fn parse_min_record(bytes: &[u8]) -> Option<Record> {
let p = bytes.as_ptr();
#[allow(clippy::cast_ptr_alignment)]
let p32 = p as *const i32;
let s = unsafe { std::slice::from_raw_parts(p32, bytes.len() / 4) };
let s = unsafe { std::slice::from_raw_parts(p32, bytes.len() / 3) };

if s.len() < 5 {
if s.len() < 4 {
return None;
}
let ptr = s.as_ptr();
let ints = unsafe { std::slice::from_raw_parts(ptr, 5) };
let ints = unsafe { std::slice::from_raw_parts(ptr, 4) };
if ints[0] != DISPATCH_MINIMAL_TOKEN {
return None;
}
Some(Record {
promise_id: ints[1],
op_id: ints[2],
arg: ints[3],
result: ints[4],
op_id: ints[1],
arg: ints[2],
result: ints[3],
})
}

#[test]
fn test_parse_min_record() {
let buf = vec![
0xFE, 0xCA, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0, 3, 0, 0, 0, 4, 0, 0, 0,
];
let buf = vec![0xFE, 0xCA, 0, 0, 1, 0, 0, 0, 2, 0, 0, 0, 3, 0, 0, 0];
assert_eq!(
parse_min_record(&buf),
Some(Record {
promise_id: 1,
op_id: 2,
arg: 3,
result: 4,
op_id: 1,
arg: 2,
result: 3,
})
);

Expand All @@ -89,8 +79,7 @@ pub fn dispatch_minimal(
state: &ThreadSafeState,
mut record: Record,
zero_copy: Option<PinnedBuf>,
) -> Op {
let is_sync = record.promise_id == 0;
) -> CoreOp {
let min_op = match record.op_id {
OP_READ => ops::read(record.arg, zero_copy),
OP_WRITE => ops::write(record.arg, zero_copy),
Expand All @@ -115,11 +104,7 @@ pub fn dispatch_minimal(
state.metrics_op_completed(buf.len());
Ok(buf)
}));
if is_sync {
Op::Sync(fut.wait().unwrap())
} else {
Op::Async(fut)
}
Op::Async(fut)
}

mod ops {
Expand Down
14 changes: 14 additions & 0 deletions cli/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,20 @@ pub fn no_buffer_specified() -> DenoError {
new(ErrorKind::InvalidInput, String::from("no buffer specified"))
}

pub fn no_async_support() -> DenoError {
new(
ErrorKind::NoAsyncSupport,
String::from("op doesn't support async calls"),
)
}

pub fn no_sync_support() -> DenoError {
new(
ErrorKind::NoSyncSupport,
String::from("op doesn't support sync calls"),
)
}

#[derive(Debug)]
pub enum RustOrJsError {
Rust(DenoError),
Expand Down
3 changes: 2 additions & 1 deletion cli/msg.fbs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ enum ErrorKind: byte {
OpNotAvaiable,
WorkerInitFailed,
UnixError,
NoAsyncSupport,
NoSyncSupport,
ImportMapError,
}

Expand All @@ -153,7 +155,6 @@ enum MediaType: byte {
}

table Base {
cmd_id: uint32;
sync: bool = false;
error_kind: ErrorKind = NoError;
error: string;
Expand Down
Loading

2 comments on commit dc60fe9

@ry
Copy link
Member

@ry ry commented on dc60fe9 Jun 14, 2019

Choose a reason for hiding this comment

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

This commit seems to have caused a minor (but hard won) regression in the http throughput benchmark:

Screen Shot 2019-06-14 at 12 06 17 PM

I will probably wait for a bit more data, but if this persists, I will revert.

@afinch7
Copy link
Contributor Author

@afinch7 afinch7 commented on dc60fe9 Jun 14, 2019

Choose a reason for hiding this comment

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

I forgot to put a comment in my pr about this. It might be something that can be resolved with some optimizations. I think that tweaking the size of the shared buffer might improve performance for starters. Also oddly for some reason this doesn't seem to affect the deno_http bench.

Please sign in to comment.