bench: add simple benchmark #253

nechaido · 2017-07-11T17:57:40Z

No description provided.

lundibundi · 2017-07-14T13:57:36Z

@metarhia/jstp-core After playing around with it for a bit I found out that our server doesn't handle highload at all and just drops with OOM, maybe should we file an issue and think for some kinds of measures against this?

lundibundi

@nechaido Also a few things:

it tests only ipc, but I think it's better to add 'socket' as first parameter to allow to specify either file or port number (it seems to be relatively easy)
if it will be possible to specify the file for ipc it should only be a name (file will always be in '/tmp/' folder) and we should also check if file not already exist to not overwrite any user data.
the bench doesn't seem to end is the server has failed, this should probably be fixed
the client also failed with the (after connection failure it was not removed from the pool, hence we've got 'undefined whatever' errors)

belochub · 2017-07-14T16:05:04Z

@lundibundi, it is impossible to overwrite user's data by listening on a socket which is an existing file, listen just won't permit the creation of the socket in that case.
Also, benchmark always removes socket file if it finishes correctly, and there is no way you can remove the file after crashing with OOM error, so it does not solve this problem. We can probably add a listener removing the file to SIGINT event, it will help in cases when the benchmark is stopped manually.

lundibundi · 2017-07-14T16:20:22Z

@belochub Ok, removed that point, it's not necessary to find ways to remove the file if server crashed with OOM, although SIGINT should probably be handled.

nechaido · 2017-07-19T12:29:35Z

@lundibundi added error and SIGINT handling.
Could not connect to the server: Error: connect EAGAIN /tmp/jstp_benchmark_ipc is a jstp error, which will be fixed by @belochub.
As for now - do not open to many connections per a client, 10 seems to be ok.

lundibundi · 2017-07-19T14:54:25Z

Adding OOM error screenshots just for reference.

For ipc connectinon:
For net socket:

lundibundi

So what about allowing to specify either filename or port to be able to test both ipc and net?

lundibundi · 2017-07-19T18:40:57Z

benchmark/run.js

+}
+
+function terminate() {
+  console.log(`


As I see those line-breaks were intentional, but maybe it's better to just use '\n'?

@lundibundi, how is that better?
As I see this, both of this options are completely equal, meaning that it is just a style preference, and we don't have a linter rule for this case, thus it is okay to leave it this way.

Yep, I didn't insist on making it that way.

@lundibundi
IMHO

console.log(` Benchmark is being terminated due to an error or signal termination `);

looks better then:

console.log( '\nBenchmark is being terminated due to an error or signal termination\n' );

This one is even better in my opinion:

const message = '\nBenchmark is being terminated due to an error or signal termination\n'; console.log(message);

IMHO

lundibundi · 2017-07-19T18:43:22Z

benchmark/worker

@@ -0,0 +1,95 @@
+'use strict';


No .js in filename.

nechaido · 2017-07-20T09:28:40Z

@lundibundi

So what about allowing to specify either filename or port to be able to test both ipc and net?

I don't like the idea, because it will only show, potential performance of JSTP over TCP. If we decide to allow user specify transport - we should allow to choose any transport.

I'd like to leave this PR this way, and add such a feature in future.

nechaido · 2017-07-20T11:55:09Z

@belochub @aqrln ping.

lundibundi

LGTM. I guess we should still add functionality to bench different transports in the follow up PRs.

belochub · 2017-07-20T12:45:43Z

benchmark/run.js

+    setTimeout(() => {
+      if (!serverExited) {
+        server.kill('SIGKILL');
+        console.log('Master processwas not able to close server gracefully');


Missing a space here.

nechaido · 2017-07-24T18:42:11Z

@belochub @aqrln ping.

aqrln

LGTM modulo comments.

aqrln · 2017-07-26T16:00:32Z

benchmark/run.js

+  size: argumentSize,
+} = args;
+
+


Nit: extra line break.

aqrln · 2017-07-26T16:03:56Z

benchmark/run.js

+const workers = new Array(workersAmount);
+const workersExited = new Array(workersAmount);
+
+const results = new Array(workersAmount);


I don't care much, but it feels like this line and the previous one should have been swapped.

aqrln · 2017-07-26T16:11:37Z

benchmark/server.js

+const server = jstp.net.createServer([app]);
+server.maxConnections = maxConnections;
+
+const socket = '/tmp/jstp_benchmark_ipc';


This won't work on Windows at all (where named pipes are created inside a special namespace in the VFS), and maybe on some Unix flavors too (it isn't correct to rely on /tmp/ being available, that's what os.tmpdir() is for). I am okay with landing this as is now, because the benchmark is not exposed to end users, but it should be fixed later. Aside from that, a hardcoded filename is prone to race conditions.

@aqrln, what kind of race conditions?

consider multiple benchmarks run simultaneously

aqrln · 2017-07-26T16:12:27Z

benchmark/worker.js

+function connect(socket) {
+  let connected = 0;
+  const createConnection = (index) => {
+    jstp.net.connectAndInspect('app', null, ['iface'], socket,


Nit: I'd put the arguments on a new line.

aqrln · 2017-07-26T16:12:43Z

benchmark/worker.js

+  for (let i = 0; i < connections.length; i++) {
+    createConnection(i);
+  }
+


Nit: unnecessary empty line.

aqrln · 2017-07-26T16:14:17Z

benchmark/worker.js

+    responseTimesHR[i] = new Array(requests);
+  }
+  let responses = 0;
+  let startTimeHR = new Array(2);


Can you please remove = new Array(2) (or assign the variable a null value)?

nechaido · 2017-07-27T11:32:30Z

@belochub ping.

belochub · 2017-07-27T12:08:16Z

benchmark/run.js

+      if (!serverExited) {
+        server.kill('SIGKILL');
+        console.log('Master process was not able to close server gracefully');
+        fs.unlinkSync('/tmp/jstp_benchmark_ipc');


You forgot to change the path here.

belochub · 2017-07-27T15:57:13Z

benchmark/run.js

+let workersConnected = 0;
+let workersFinished = 0;
+
+let becnhStartedHR;


Typo in word "bench" here.

belochub · 2017-07-27T18:02:36Z

benchmark/run.js

+    setTimeout(() => {
+      if (!serverExited) {
+        server.kill('SIGKILL');
+        console.log('Master process was not able to close server gracefully');


It'll be better to replace console.log with console.warn here.

PR-URL: #253 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub · 2017-07-29T15:42:49Z

Landed in 5bf3944.

PR-URL: #253 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

nechaido added the benchmark label Jul 11, 2017

nechaido force-pushed the benchmark branch from 54bd846 to be25259 Compare July 11, 2017 18:38

nechaido requested review from belochub, aqrln and lundibundi July 11, 2017 18:39

lundibundi reviewed Jul 14, 2017

View reviewed changes

nechaido force-pushed the benchmark branch from be25259 to 27fbbf1 Compare July 17, 2017 13:09

lundibundi reviewed Jul 19, 2017

View reviewed changes

lundibundi approved these changes Jul 20, 2017

View reviewed changes

belochub reviewed Jul 20, 2017

View reviewed changes

belochub mentioned this pull request Jul 21, 2017

lib,parser: optimize parser and socket transport #254

Closed

nechaido force-pushed the benchmark branch from 0a30f03 to edb28df Compare July 24, 2017 14:51

aqrln approved these changes Jul 26, 2017

View reviewed changes

belochub reviewed Jul 27, 2017

View reviewed changes

nechaido force-pushed the benchmark branch from 2521110 to 222131e Compare July 27, 2017 15:37

belochub reviewed Jul 27, 2017

View reviewed changes

benchmark/run.js Outdated

let workersConnected = 0;

let workersFinished = 0;

let becnhStartedHR;

Copy link

Member

belochub Jul 27, 2017

Choose a reason for hiding this comment

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

Typo in word "bench" here.

belochub reviewed Jul 27, 2017

View reviewed changes

belochub approved these changes Jul 28, 2017

View reviewed changes

bench: add simple benchmark

4e483b9

nechaido force-pushed the benchmark branch from c83d58c to 4e483b9 Compare July 28, 2017 13:17

belochub pushed a commit that referenced this pull request Jul 29, 2017

bench: add simple benchmark

5bf3944

PR-URL: #253 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub closed this Jul 29, 2017

belochub deleted the benchmark branch July 29, 2017 15:42

belochub mentioned this pull request Jul 29, 2017

lib: add handling of EAGAIN error on connection #281

Closed

belochub pushed a commit that referenced this pull request Jan 22, 2018

bench: add simple benchmark

2ccdc66

PR-URL: #253 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub pushed a commit that referenced this pull request Jan 22, 2018

bench: add simple benchmark

e90982f

PR-URL: #253 Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>

belochub mentioned this pull request Jan 22, 2018

v1.0.0 proposal #311

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bench: add simple benchmark #253

bench: add simple benchmark #253

nechaido commented Jul 11, 2017

lundibundi commented Jul 14, 2017

lundibundi left a comment •

edited

Loading

belochub commented Jul 14, 2017

lundibundi commented Jul 14, 2017

nechaido commented Jul 19, 2017

lundibundi commented Jul 19, 2017

lundibundi left a comment

lundibundi Jul 19, 2017

belochub Jul 19, 2017

lundibundi Jul 19, 2017

nechaido Jul 20, 2017

lundibundi Jul 20, 2017 •

edited

Loading

lundibundi Jul 19, 2017

nechaido commented Jul 20, 2017 •

edited

Loading

nechaido commented Jul 20, 2017 •

edited

Loading

lundibundi left a comment

belochub Jul 20, 2017

nechaido commented Jul 24, 2017

aqrln left a comment

aqrln Jul 26, 2017

aqrln Jul 26, 2017

aqrln Jul 26, 2017

belochub Jul 27, 2017

aqrln Jul 27, 2017 •

edited

Loading

aqrln Jul 26, 2017

aqrln Jul 26, 2017

aqrln Jul 26, 2017

nechaido commented Jul 27, 2017

belochub Jul 27, 2017

belochub Jul 27, 2017

belochub Jul 27, 2017

belochub commented Jul 29, 2017

bench: add simple benchmark #253

bench: add simple benchmark #253

Conversation

nechaido commented Jul 11, 2017

lundibundi commented Jul 14, 2017

lundibundi left a comment • edited Loading

Choose a reason for hiding this comment

belochub commented Jul 14, 2017

lundibundi commented Jul 14, 2017

nechaido commented Jul 19, 2017

lundibundi commented Jul 19, 2017

lundibundi left a 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

lundibundi Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido commented Jul 20, 2017 • edited Loading

nechaido commented Jul 20, 2017 • edited Loading

lundibundi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nechaido commented Jul 24, 2017

aqrln left a 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

aqrln Jul 27, 2017 • 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

nechaido commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

belochub commented Jul 29, 2017

lundibundi left a comment •

edited

Loading

lundibundi Jul 20, 2017 •

edited

Loading

nechaido commented Jul 20, 2017 •

edited

Loading

nechaido commented Jul 20, 2017 •

edited

Loading

aqrln Jul 27, 2017 •

edited

Loading