Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

optimization: {active, true} only set when needed #482

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

DBarney commented Mar 28, 2013

Currently all messages sent to a websocket_handler process will trigger
the option {active, true} to be set on the socket. This causes
unnecessary time to be spent setting an option that is already set.
This is especially true for websockets that primarily listen for
messages from other processes and forward them to the connected client.
{active, true} only needs to be set on the socket after a data packet
has been received, and once at the start of the wesocket to "prime the
pump".

Owner

essen commented Mar 28, 2013

{active, once} you mean?

DBarney commented Mar 29, 2013

Yes I did. Would you like me to correct the commit message and then create a new pull request? Or just create an empty commit with the corrected message?

Owner

essen commented Mar 29, 2013

If you amend the commit and push -f it will update this PR. Feel free to update. :)

DBarney commented Apr 4, 2013

OK I amended the commit and pushed. Thanks Essen.

Owner

essen commented Apr 12, 2013

There's a spacing issue line 181.

I'm OK with this but we probably want to disable active, once and flush any message if the handler decided to shutdown in websocket_data.

@DBarney DBarney optimization: {active, once} only set when needed
Currently all messages sent to a websocket_handler process will trigger
the option {active, once} to be set on the socket. This causes
unnecessary time to be spent setting an option that is already set.
This is especially true for websockets that primarily listen for
messages from other processes and forward them to the connected client.
{active, once} only needs to be set on the socket after a data packet
has been received, and once at the start of the wesocket to "prime the
pump".
9dd0028

DBarney commented Apr 12, 2013

Ok I fixed the spacing issue, I also added in the flushing of any messages that could have arrived as a result of the Socket being in {active, once}. The behavior is called in websocket_close because there are multiple places where the Socket is put in {active, once} and then could be closed: web socket_data, websocket_payload, websocket_payload_loop, handler_call, websocket_dispatch.

Thanks Essen.

Owner

essen commented Apr 12, 2013

I'll run it against autobahn and see if it improves anything.

Owner

essen commented Apr 12, 2013

Autobahn doesn't seem to say it's better. Do you have measurements that show it's improving things?

DBarney commented Apr 12, 2013

yes just a second, I have to run them again.

DBarney commented Apr 12, 2013

I connected a websocket client to cowboy, both before and after the patch, and sent it 1000 messages. The messages in this case aren't acted on, but that is only to highlight the differences in speed between the two versions.

So here are the results: https://gist.github.com/DBarney/5374070

Granted not a huge improvement in speed because it is already very fast.

But : "This is especially true for websockets that primarily listen for messages from other processes and forward them to the connected client." and that is what the service that I am optimizing does. Which is why this was identified as an area we could squeeze extra performance out of our servers.

Thanks Essen.

Owner

essen commented Apr 12, 2013

How do I read this? I don't understand the values, though I guess it says it calls setopts less (we already knew that).

DBarney commented Apr 12, 2013

You have never used fprof? If you haven't you really should look into it, its built into erlang and its really awesome.

here is how to interpret the results:
http://www.erlang.org/doc/man/fprof.html#id79359

Here is a summary:

  • without the patch 62.619 milliseconds per 1000 messages
  • with the patch 16.963 milliseconds per 1000 messages
Owner

essen commented Apr 12, 2013

I used fprof sometimes. I can't use that for anything more than "this seems to be the right thing to do" though. It only says we spend less time in setopts (we know!).

Can you measure the time it takes for the client to send everything (and get a final reply possibly) from the client side instead?

DBarney commented Apr 12, 2013

sure, just give me a minute and I'll get that for you.

Owner

essen commented Apr 25, 2013

Any news?

Owner

essen commented May 31, 2013

Hi. Still waiting hoping for your message. :)

Owner

essen commented Aug 27, 2013

Hi.

Any update on this?

DBarney commented Jan 22, 2014

Sorry about not getting back for a long time, I have been really busy this last year and just haven't gotten around to writing up a full benchmark to show the difference between the two implementations.

But thinking about it now, the improvement is probably close to nothing. There are (probably) other areas that can be speed up before a micro optimization like this one needs to be even thought about.

So I'm going to close this for now.

@DBarney DBarney closed this Jan 22, 2014

Owner

essen commented Jan 22, 2014

No problem, thanks for trying!

Also the new {active, N} option is gonna be a more interesting optimization when we can start using it.

DBarney commented Jan 24, 2014

Yeah it will probably even be a better solution then doing {active, once} every time a new data packet it needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment