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.

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

benchmark: chunky http client benchmark variation and server #228

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 104 additions & 0 deletions benchmark/net/chunky_http_client.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

Style: the convention is to use single quotes. There is usually a blank line after 'use strict'.

// test HTTP throughput in fragmented header case
var common = require('../common.js');
var net = require('net');
var test = require('../../test/common.js');

var bench = common.createBenchmark(main, {
// len: [1, 4, 8, 16, 32, 64, 128],
Copy link
Member

Choose a reason for hiding this comment

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

Style: don't leave commented out code in.

len: [1],
num: [5, 50, 500, 2000],
type: ['send'],
}
)
Copy link
Member

Choose a reason for hiding this comment

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

Style: });



function main(conf) {
var len;
var num;
var type;
num = +conf.num;
len = +conf.len;
type = conf.type;
Copy link
Member

Choose a reason for hiding this comment

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

Style: declaration and assignment normally go on the same line. You can use let or const if you want.

var todo = [];
var headers = [];
// Chose 7 because 9 showed "Connection error" / "Connection closed"
// An odd number could result in a better length dispersion.
for (var i = 7; i <= 7*7*7; i *= 7)
headers.push(Array(i + 1).join('o'));

function WriteWithCRLF(line) {
todo.push(line + '\r\n');
}

function WriteHTTPHeaders(channel, has_keep_alive, extra_header_count) {
todo = []
WriteWithCRLF('GET / HTTP/1.1');
WriteWithCRLF('Host: localhost');
WriteWithCRLF('Connection: keep-alive');
WriteWithCRLF('Accept: text/html,application/xhtml+xml,' +
'application/xml;q=0.9,image/webp,*/*;q=0.8');
Copy link
Member

Choose a reason for hiding this comment

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

Style: string should line up with the one above it.

WriteWithCRLF('User-Agent: Mozilla/5.0 (X11; Linux x86_64) ' +
'AppleWebKit/537.36 (KHTML, like Gecko) ' +
'Chrome/39.0.2171.71 Safari/537.36');
WriteWithCRLF('Accept-Encoding: gzip, deflate, sdch');
WriteWithCRLF('Accept-Language: en-US,en;q=0.8');
for (var i = 0; i < extra_header_count; i++) {
// Utilize first three powers of a small integer for an odd cycle and
// because the fourth power of some integers overloads the server.
WriteWithCRLF('X-Header-' + i + ': ' + headers[i%3]);
}
WriteWithCRLF('');
todo = todo.join('');
Copy link
Member

Choose a reason for hiding this comment

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

I think you can replace WriteWithCRLF() with todo.push(), then do todo = todo.join('\r\n')?

// Using odd numbers in many places may increase length coverage.
var chunksize = 37;
for (i = 0; i < todo.length; i+=chunksize) {
var cur = todo.slice(i,i+chunksize);
Copy link
Member

Choose a reason for hiding this comment

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

Style: spaces before and after operators (here and elsewhere.)

channel.write(cur);
}
}

var success = 0, failure = 0, min = 10, size = 0;
var mod = 317, mult = 17, add = 11;
Copy link
Member

Choose a reason for hiding this comment

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

Style: prefer one declaration per line.

var count = 0;
var PIPE = test.PIPE;
var socket = net.connect(PIPE, function() {
bench.start();
WriteHTTPHeaders(socket, 1, +conf.len);
socket.setEncoding('utf8')
socket.on('data', function(d) {
var did = false;
var pattern = 'HTTP/1.1 200 OK\r\n';
if ((d.length === pattern.length && d === pattern) ||
(d.length > pattern.length &&
d.slice(0, pattern.length) === pattern)) {
success += 1;
did = true;
} else {
pattern = 'HTTP/1.1 '
if ((d.length === pattern.length && d === pattern) ||
(d.length > pattern.length &&
d.slice(0, pattern.length) === pattern)) {
failure += 1;
did = true;
}
}
size = (size * mult + add) % mod;
if (did) {
count += 1;
if (count === num) {
bench.end(count);
} else {
WriteHTTPHeaders(socket, 1, min + size);
}
}
});
socket.on('close', function() {
console.log('Connection closed');
});

socket.on('error', function() {
console.log('Connection error');
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Style: indentation seems off here.

}
47 changes: 47 additions & 0 deletions benchmark/net/http_server_for_chunky_client.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
"use strict";
var path = require('path');
var http = require('http');
var fs = require('fs');
var spawn = require('child_process').spawn;
var common = require('../common.js')
var test = require('../../test/common.js')
var pep = path.dirname(process.argv[1])+'/chunky_http_client.js';
var PIPE = test.PIPE;

var server;
try {
fs.unlinkSync(PIPE);
} catch (e) { /* ignore */ }

server = http.createServer(function(req, res) {
res.writeHead(200, { 'content-type': 'text/plain',
'content-length': '2' });
res.end('ok');
});

server.on('error', function(err) {
console.error('Error:');
console.error(err);
});

try {
server.listen(PIPE, 'localhost');
Copy link
Member

Choose a reason for hiding this comment

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

'localhost' is not needed and you may have a race condition here. The child process should be started from inside the 'listening' event listener.

} catch(e) {
console.error('Error:');
console.error(e);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by your use of try/catch around .listen(). Mind elaborating?

Copy link
Author

Choose a reason for hiding this comment

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

Happy to Trevor and BTW thanks for the guidance and additional comments they are quite helpful. Even though we do an unlinkSync(PIPE) above, it seems to be no guarantee against another mistaken process interfering accidentally. I seem to remember that it is possible to bind to a port with an AF_INET address exclusively or not a la SO_REUSEADDR / etc and I had assumed there was some similar risks with AF_UNIX family that another process might accidentally do. Please let me know if you believe otherwise (e.g. call cannot ever fail) and I would like to find out more as well and simplify this code.

Copy link
Member

Choose a reason for hiding this comment

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

If the socket path is taken, then there's nothing you can do anyway. Best to just let the script fail.


var child = spawn(process.execPath, [pep], { });
child.on('error', function(err) {
console.error('spawn error.');
console.error(err);
});
Copy link
Member

Choose a reason for hiding this comment

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

If you're not going to handle them, you should let 'error' events bubble up.


child.stdout.on('data', function (data) {
process.stdout.write(data);
});
Copy link
Member

Choose a reason for hiding this comment

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

child.stdout.pipe(process.stdout) is shorter and more efficient. You can also spawn the process with 'inherit' for stdout.


child.on('exit', function (exitCode) {
console.error('Child exited with code: ' + exitCode);
process.exit(0);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, tests and benchmarks terminate without calling process.exit(). process.exit() sometimes hides bugs in the script.

});