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.

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

error handling in minimal dispatch #3176

Merged
merged 8 commits into from
Oct 24, 2019
Merged

error handling in minimal dispatch #3176

merged 8 commits into from
Oct 24, 2019

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Oct 21, 2019 •
edited
Loading

Towards #3117

A little prototype of error handling in minimal dispatch. I expect it to be slow but I want to start a discussion.

CC @ry

@bartlomieju bartlomieju force-pushed the refactor-error_handling_in_dispatch branch from 3212df5 to ecc630e Compare October 21, 2019 22:03
@ry
Copy link
Member

ry commented Oct 22, 2019

@bartlomieju Does this have an effect on the deno_tcp http req/sec benchmark?

@bartlomieju
Copy link
Member Author

bartlomieju commented Oct 22, 2019 •
edited
Loading

@ry I get around 88k req/sec instead of 95k. I need to work on it a bit more, started getting some panics locally :/

EDIT: Yikes, I put wrong numbers. Actually I'm getting around 88k req/s in deno_tcp on master branch. After fixes I get around 90k req/s with this PR. ¯_(ツ)_/¯

@bartlomieju
Copy link
Member Author

this PR

  "req_per_sec": {
    "deno_tcp": 50138, 
    "deno_core_single": 62672, 
    "node_proxy_tcp": 14730, 
    "deno_proxy_tcp": 14773, 
    "deno_core_multi": 61024, 
    "deno_tcp_current_thread": 47704, 
    "hyper": 54209, 
    "node_tcp": 33282, 
    "deno_http": 13311, 
    "node_http": 17620, 
    "deno_proxy": 1014, 
    "node_proxy": 2683
  }, 
  "max_latency": {
    "deno_tcp": 8.26, 
    "deno_core_single": 1.31, 
    "node_proxy_tcp": 3.06, 
    "deno_proxy_tcp": 3.82, 
    "deno_core_multi": 5.49, 
    "deno_tcp_current_thread": 1.96, 
    "hyper": 3.36, 
    "node_tcp": 2.72, 
    "deno_http": 31.25, 
    "node_http": 4.82, 
    "deno_proxy": 18.39, 
    "node_proxy": 9.86
  },

master

  "req_per_sec": {
    "deno_tcp": 55549, 
    "deno_core_single": 59915, 
    "node_proxy_tcp": 15679, 
    "deno_proxy_tcp": 18249, 
    "deno_core_multi": 62742, 
    "deno_tcp_current_thread": 55106, 
    "hyper": 54423, 
    "node_tcp": 35625, 
    "deno_http": 13972, 
    "node_http": 17561, 
    "deno_proxy": 1096, 
    "node_proxy": 2846
  }, 
  "max_latency": {
    "deno_tcp": 5.32, 
    "deno_core_single": 1.16, 
    "node_proxy_tcp": 2.06, 
    "deno_proxy_tcp": 3.29, 
    "deno_core_multi": 6.15, 
    "deno_tcp_current_thread": 2.04, 
    "hyper": 2.66, 
    "node_tcp": 3.26, 
    "deno_http": 38.37, 
    "node_http": 3.76, 
    "deno_proxy": 16.86, 
    "node_proxy": 9.06
  },

@bartlomieju bartlomieju reopened this Oct 24, 2019
@bartlomieju
Copy link
Member Author

@ry, how about we land this PR so we can remove panics for malformed control buffers and work on any potential perf problems down the road?

ry
ry reviewed Oct 24, 2019
View reviewed changes
cli/js/dispatch_minimal.ts
result: buf32[2]
arg,
result,
err
};
}
Copy link
Member

Choose a reason for hiding this comment

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

This code is very much in the hot path for our most important benchmark. I'm all for landing this to see what happens, but I will revert if it causes a noticeable slow down.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I agree completely. I will try to optimize it further, but we can't leave out proper error handling (which is long overdue anyway)

@ry
Copy link
Member

ry commented Oct 24, 2019

@bartlomieju Sounds good - please add a unit test for the method I pointed out and I'll land it.

ry
ry reviewed Oct 24, 2019
View reviewed changes
cli/js/dispatch_minimal.ts Outdated Show resolved Hide resolved
@bartlomieju bartlomieju force-pushed the refactor-error_handling_in_dispatch branch from 977ac04 to f5e1fee Compare October 24, 2019 15:44
@bartlomieju
Copy link
Member Author

bartlomieju commented Oct 24, 2019 •
edited
Loading

@ry it looks like sccache is acting up on Windows. Did you have any problems like that recently?

EDIT: Probable cause described by @chrmoritz in #3194 (comment)

@bartlomieju
Copy link
Member Author

@ry ready

ry
ry reviewed Oct 24, 2019
View reviewed changes
cli/ops/dispatch_minimal.rs
error_message: "Error".to_string().as_bytes().to_owned(),
};
let buf: Buf = err_record.into();
assert_eq!(buf, expected.into_boxed_slice());
Copy link
Member

Choose a reason for hiding this comment

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

👍

ry
ry approved these changes Oct 24, 2019
View reviewed changes
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM

@ry ry merged commit 492b87d into denoland:master Oct 24, 2019
@kevinkassimo
Copy link
Contributor

The slowdown from this PR seems quite apparent from benchmark:

Screenshot of Deno

@bartlomieju bartlomieju deleted the refactor-error_handling_in_dispatch branch November 10, 2019 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ry ry ry approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bartlomieju @ry @kevinkassimo