search

stompgem / stomp

A ruby gem for sending and receiving messages from a Stomp protocol compliant message queue. Includes: failover logic, ssl support.
http://stomp.github.com
Apache License 2.0
152 stars 80 forks source link

Turned on the TCP_NODELAY socket option (disables Nagle's algorithm) #76

Closed ismith closed 10 years ago

ismith commented 10 years ago

Tests show an 8-20 fold improvement in throughput. Messages under 1500 total bytes benefit the most. Even with messages of 10K in size there's still a 30% improvement in throughput; that's a lot. Tested with JRuby 1.7.3 and Ruby EE 1.9.3 on RHEL 6.1 against ActiveMQ 5.8.0 (also running on RHEL 6.1).

Default is true, but can TCP_NODELAY can be turned off by putting :tcp_nodelay => false in the parameters hash.

(https://gist.github.com/ppaul/6540400, #8)

rtyler commented 10 years ago

This looks good to me, but then again, what do I know. Perhaps @gmallard should weigh in :)

gmallard commented 10 years ago

I have no objections to this. Even though no evidence has been presented that performance advantages actually exist for some workloads.

The change has very low probability of breaking existing users. No one has asked for it since 2005.

Following the philosophy of "do not break existing public APIs", perhaps a default of 'false' for this option might be considered.

But I am OK with either true/false for the default.

PaulGale commented 10 years ago

Even though no evidence has been presented that performance advantages actually exist for some workloads. I have evidence but I cannot be bothered to offer it up given your predisposition for being a right-fighter.

Following the philosophy of "do not break existing public APIs", perhaps a default of 'false' for this option might be considered. The parameter's value cannot 'break' the API even if it worsened performance, which it doesn't. What definition of 'break' are you using?

There's no point following a philosophy unless you both understand it and know why you're following it. I hear the platitude "don't break the API" cited often but I'm left wondering whether folks really understand what they're saying.

Preserving bad and flawed abstractions so as to avoid causing upset to some users is crazy talk. That's not what it's about. Planes will not fall out of the sky and people will not die. Removing the positional constructors, for example, is such a trivial change that it would take any developer a few minutes to change their code, however the pay off for the gem would be significant. Developers get angry when large scale changes are made and they're either not documented or poorly documented; therefore document the heck out of them via release notes. Most developers would agree that making sweeping changes to an API that don't add much value is annoying. However, what annoys developers more is when requests for changes to a bad or flawed API are rejected on the basis of the abused justification "it will break the public API". That's not a concern about compatibility, that's sloppiness.

Recently we've been using a third-party commercial Java API that went through a dot release. The company in question made substantial changes to its public API. As a business they have a huge incentive not to upset paying customers. However, we didn't pout just because a change was made to the API; the changes made the API so much better. A couple of hours later we were back in the saddle and the code was much better for it.

Thanks, Paul

On Mon, Sep 23, 2013 at 7:13 PM, Guy M. Allard notifications@github.comwrote:

I have no objections to this. Even though no evidence has been presented that performance advantages actually exist for some workloads.

The change has very low probability of breaking existing users. No one has asked for it since 2005.

Following the philosophy of "do not break existing public APIs", perhaps a default of 'false' for this option might be considered.

But I am OK with either true/false for the default.

— Reply to this email directly or view it on GitHubhttps://github.com/stompgem/stomp/pull/76#issuecomment-24963170 .

ismith commented 10 years ago

@gmallard I think the potential performance improvements are worth making the default value true, although I did consider defaulting to false since, as you note, it is a change in behavior.

@ppaul If you have evidence that's easy to share, I'd appreciate you leaving it in a comment here for posterity. No need to dredge up old conflicts. Regardless, though, I'm going to go ahead and merge this tomorrow.