First pass at support for TCP servers and clients. #884

ry · 2018-10-03T05:55:42Z

Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.

Fixes #725

ghost · 2018-10-03T15:52:55Z

Cool, finally something that does IO and can be benchmarked. Looking forward to testing this out.

From my benchmarking of Node.js it really doesn't matter if you're looking at HTTP or WebSocket performance, both are capped at fairly low req/sec or messages/sec by what seems to be the TCP wrappers. The project "websockets/ws" perform about identical amount of messages per second as Node.js does HTTP req/sec.

If you want higher perf. with Deno this thing will be critical to solve at much higher efficiency than for Node.js as it basically caps everything else (database transfers, messaging, http, etc). Well, Node.js does a good job for big sends (because it is capping out the kernel) but for small sends it really falls behind with 1/10th the performance of what you could get on the same system.

Let's make sure to follow this up with at least a simple ping/pong benchmark to measure the theoretical maximum Deno apps can expect to get when doing network IO.

ry · 2018-10-03T18:31:51Z

@alexhultman Regarding benchmarks - probably a good one would be to start an echo TCP server and pipe 100M of data thru it using netcat. Here's an example that works with this branch:

import * as deno from "deno";
let listener = deno.listen("tcp", "127.0.0.1:4500");
listener.accept().then(async (conn) => {
  await deno.copy(conn, conn);
  conn.close();
  listener.close();
});

Then in another terminal you run:

> time head -c 100000000 /dev/zero | nc localhost 4500

real	0m38.465s
user	0m0.472s
sys	0m3.955s

There is likely much low hanging fruit here. I've been waiting to have such a benchmark before attempting to optimize. For example, just by increasing the buffer inside copy() to 64k, it improves quite a bit:

> time head -c 100000000 /dev/zero | nc localhost 4500

real	0m7.490s
user	0m0.480s
sys	0m4.352s

Also these numbers are for the debug build, not release.

ghost · 2018-10-03T18:45:40Z

Piping 100 MB of data is kind of a pointless test. All servers will perform exactly the same in that test, because you are not testing anything user space, everything is going to be bottlenecked by the kernel space.

It doesn't matter if you pick a web server written in Ruby on Rails or C in that case. You are bottlenecking on syscalls because all the process ends up doing is just passing huge blocks of memory to the send syscall.

What I had in mind was small sends used in REST services (think sub 16kb) and WebSocket messaging (think sub 1kb) and similar. That's where you will see a difference between the servers.

Small sends have a much higher ratio of user space involvement than huge static sends.

ry · 2018-10-03T18:55:49Z

everything is going to be bottlenecked by the kernel space.

Hopefully! That's the point of the benchmark. The code path for that echo server is not trivial - it includes

All of these are potential bottlenecks.

ghost · 2018-10-03T18:58:31Z

Add both then. I get the exact same perf. in Python, Node.js and C when doing large sends. They all consume almost 0% user space CPU time and 100% kernel space CPU time. Small sends swap those numbers.

It is completely impossible to outperform Node.js in large send throughput on Linux, unless you replace the entire kernel with some bypass user space TCP stack.

ghost · 2018-10-03T19:01:56Z

I have a graph of the thing here: https://raw.githubusercontent.com/uNetworking/uSockets/master/misc/http.png

Everything over 50mb is completely kernel bottlenecked. You can clearly see that Node.js has serious issues with small sends.

ry · 2018-10-03T19:11:13Z

@alexhultman I encourage you to outline your desired benchmark in the issues, I think it makes a lot of sense.

Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.

piscisaureus · 2018-10-04T02:51:10Z

js/net.ts

+    readonly localAddr: string
+  ) {}
+
+  write(p: ArrayBufferView): Promise<number> {


I'm missing something like shutdown here. How is that done?

It's not implemented yet.
It will be added to the AsyncWrite implementation in resources.rs. There's a sub here

deno/src/resources.rs

Lines 104 to 106 in e5e7f0f

fn shutdown(&mut self) -> futures::Poll<(), std::io::Error> {

unimplemented!()

}

https://golang.org/pkg/net/#TCPConn.CloseRead
Following golang I will surface it as closeRead() and closeWrite()

I've updated the source code to include a stubbed method.

piscisaureus · 2018-10-04T02:54:08Z

js/net.ts

+ *
+ * See dial() for a description of the network and address parameters.
+ */
+export function listen(network: Network, address: string): Listener {


I find "network" very confusing. Why not "protocol" like everybody else and their cat?

It comes from Golang https://golang.org/pkg/net/#Listen

I see. Still a bad name.
"unix packet" has nothing to do with network.
Soon well have "websocket" which is also not a network.

piscisaureus · 2018-10-04T02:56:12Z

js/net.ts

+ *   dial("udp", "[fe80::1%lo0]:53")
+ *   dial("tcp", ":80")
+ */
+export async function dial(network: Network, address: string): Promise<Conn> {


Not sold on the dial name, but let's keep it for now.
However I would suggest to treat connect as a reserved method name, in case we change our mind.

piscisaureus · 2018-10-04T02:59:34Z

src/handlers.rs

+  let address = msg.address().unwrap();
+
+  Box::new(futures::future::result((move || {
+    // TODO properly parse addr


What's not proper about it?

It doesn't handle port only: ":8080"
needs tests to ensure conformance with golang.

piscisaureus · 2018-10-04T03:03:24Z

src/resources.rs

 pub struct Resource {
  pub rid: ResourceId,
 }

+impl Resource {
+  // TODO Should it return a Resource instead of net::TcpStream?


IDK, I can't see through it at this point. Let's leave it for now and refactor (if necessary) when we add other types of servers.

piscisaureus

Mostly LGTM, but I would like to hear how shutdown() fits in, or alternatively why it is not needed.

I really like that this is mostly simple additive changes and requires little refactoring. That suggests that we are getting the internal abstractions right.

- Improve fetch headers (denoland#853) - Add deno.truncate (denoland#805) - Add copyFile/copyFileSync (denoland#863) - Limit depth of output in console.log for nested objects, and add console.dir (denoland#826) - Guess extensions on extension not provided (denoland#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (denoland#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.

- Improve fetch headers (#853) - Add deno.truncate (#805) - Add copyFile/copyFileSync (#863) - Limit depth of output in console.log for nested objects, and add console.dir (#826) - Guess extensions on extension not provided (#859) - Renames: deno.platform -> deno.platform.os deno.arch -> deno.platform.arch - Upgrade TS to 3.0.3 - Add readDirSync(), readDir() - Add support for TCP servers and clients. (#884) Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.

ry requested a review from piscisaureus October 3, 2018 05:55

ry changed the title ~~Add support for TCP servers and clients.~~ First pass at support for TCP servers and clients. Oct 3, 2018

ry force-pushed the net branch 3 times, most recently from 7b2378b to 90b78e9 Compare October 3, 2018 06:12

ry mentioned this pull request Oct 3, 2018

Benchmark wanted: copy throughput #890

Closed

Add support for TCP servers and clients.

9fbbc6e

Adds deno.listen(), deno.dial(), deno.Listener and deno.Conn.

ry force-pushed the net branch from 90b78e9 to 9fbbc6e Compare October 3, 2018 22:50

piscisaureus reviewed Oct 4, 2018

View reviewed changes

Add unused but reserved op 'connect'

6d7803b

ry force-pushed the net branch from 74da364 to 85099af Compare October 4, 2018 03:25

Add closeRead() AKA shutdown(2)

6a4f638

ry force-pushed the net branch from 85099af to 6a4f638 Compare October 4, 2018 03:27

piscisaureus approved these changes Oct 4, 2018

View reviewed changes

fmt

f948d54

ry merged commit 0422b22 into denoland:master Oct 4, 2018

ry deleted the net branch October 4, 2018 03:58

ry mentioned this pull request Oct 4, 2018

v0.1.7 #902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass at support for TCP servers and clients. #884

First pass at support for TCP servers and clients. #884

ry commented Oct 3, 2018 •

edited

Loading

ghost commented Oct 3, 2018

ry commented Oct 3, 2018 •

edited

Loading

ghost commented Oct 3, 2018

ry commented Oct 3, 2018 •

edited

Loading

ghost commented Oct 3, 2018

ghost commented Oct 3, 2018

ry commented Oct 3, 2018

piscisaureus Oct 4, 2018

ry Oct 4, 2018

ry Oct 4, 2018 •

edited

Loading

ry Oct 4, 2018

piscisaureus Oct 4, 2018

ry Oct 4, 2018

piscisaureus Oct 4, 2018

piscisaureus Oct 4, 2018

ry Oct 4, 2018

piscisaureus Oct 4, 2018

ry Oct 4, 2018

piscisaureus Oct 4, 2018

piscisaureus left a comment

	fn shutdown(&mut self) -> futures::Poll<(), std::io::Error> {
	unimplemented!()
	}

First pass at support for TCP servers and clients. #884

First pass at support for TCP servers and clients. #884

Conversation

ry commented Oct 3, 2018 • edited Loading

ghost commented Oct 3, 2018

ry commented Oct 3, 2018 • edited Loading

ghost commented Oct 3, 2018

ry commented Oct 3, 2018 • edited Loading

ghost commented Oct 3, 2018

ghost commented Oct 3, 2018

ry commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ry Oct 4, 2018 • 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

piscisaureus left a comment

Choose a reason for hiding this comment

ry commented Oct 3, 2018 •

edited

Loading

ry commented Oct 3, 2018 •

edited

Loading

ry commented Oct 3, 2018 •

edited

Loading

ry Oct 4, 2018 •

edited

Loading