Performance regression in 4.1.10.0-RELEASE

[ianmcorvidae]

Hi; we (CyVerse) recently attempted to upgrade from 4.0.2.6-RELEASE to 4.1.10.0-RELEASE, and saw pretty dramatic performance degradation. After a lot of digging and some git bisecting, a52751e is the first commit with the issue. Specifically, commenting out the flush in https://github.com/DICE-UNC/jargon/blob/4.1.10.0-RELEASE/jargon-core/src/main/java/org/irods/jargon/core/connection/AbstractConnection.java#L431 (which, by the time of the release, is what's used by sendHeader -- see https://github.com/DICE-UNC/jargon/blob/4.1.10.0-RELEASE/jargon-core/src/main/java/org/irods/jargon/core/connection/IRODSMidLevelProtocol.java#L326) fixes the issue -- it seems that iRODS dislikes it when you flush the 4-byte header size without the rest of the header following it, and for some reason this makes it so that when reading in the header size of the response, it takes much longer than usual to read the header of the response from iRODS (typically, but not always, 40ms instead of 2-7ms -- compounded across a number of messages send to iRODS in the context of a single request to our services, this caused a rapid increase in response times).

I'm not sure if simply removing the flush in sendInNetworkOrder is correct though, as that's used other places - in any case it appears that there should not be a flush between the 4-byte header size and the actual XML header of the message. Perhaps it should be possible to pass a parameter to sendInNetworkOrder which tells it not to do the flush() at the end?

Let me know if I can provide more information.

[michael-conway]

excellent Ian. We'd had some reports of this. Yes, the flush behavior has been really a headache, especially trying to maintain backwards compatability. The flush behavior had been an issue with pre 4.1 iRODS, and jargon.properties did have a force flush option that was honored in the pam code. I know you all have SSL turned off, so before we work that back in we need to A/B test this with SSL turned on. The agent behavior has changed a bit with SSL on the server side, so we may need to have some version-aware logic in the next release to help avoid this. Maybe we can talk in depth at the beginning of next week? I'm travelling now so it'll be Monday earliest before I can dig into that.

…

On Wed, Apr 12, 2017 at 4:42 PM, Ian McEwen ***@***.***> wrote: Hi; we (CyVerse) recently attempted to upgrade from 4.0.2.6-RELEASE to 4.1.10.0-RELEASE, and saw pretty dramatic performance degradation. After a lot of digging and some git bisecting, a52751e <a52751e> is the first commit with the issue. Specifically, commenting out the flush in https://github.com/DICE-UNC/jargon/blob/4.1.10.0-RELEASE/ jargon-core/src/main/java/org/irods/jargon/core/connection/ AbstractConnection.java#L431 (which, by the time of the release, is what's used by sendHeader -- see https://github.com/DICE-UNC/ jargon/blob/4.1.10.0-RELEASE/jargon-core/src/main/java/org/ irods/jargon/core/connection/IRODSMidLevelProtocol.java#L326) fixes the issue -- it seems that iRODS dislikes it when you flush the 4-byte header size without the rest of the header following it, and for some reason this makes it so that when reading in the header size of the response, it takes much longer than usual to read the header of the response from iRODS (typically, but not always, 40ms instead of 2-7ms -- compounded across a number of messages send to iRODS in the context of a single request to our services, this caused a rapid increase in response times). I'm not sure if simply removing the flush in sendInNetworkOrder is correct though, as that's used other places - in any case it appears that there should not be a flush between the 4-byte header size and the actual XML header of the message. Perhaps it should be possible to pass a parameter to sendInNetworkOrder which tells it not to do the flush() at the end? Let me know if I can provide more information. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#243>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABC-LbYDGxOsATIs99SdfpUiQon3ACf2ks5rvTcagaJpZM4M78dv> .

[ianmcorvidae]

That sounds reasonable to me. I'm hoping/assuming you have SSL-enabled servers to test against, since of course I only have ours! But I'm happy to test any proposed changes within our code/against our iRODS instances, of course.

[michael-conway]

Yes. Will try to catch up next week. Thanks again for your digging, as this might help a few other folks out.

[michael-conway]

Integrating into current jargon and testing....

[dkocher]

Is this related to #234?

[ghost]

@dkocher I think it is.

[michael-conway]

I think it is as well, but want to test further before I link the two. It has to do with behavior of iRODS (various releases) on the agent side and whether the flush() is necessary before sending headers. It is mixed in with the changes to support SSL.

I will likely do a point release with the SSL, the flush, and a few other fixes and make a pass at outstanding issues to see what I can fit in this week.

[ghost]

Any updates on this issue?

[michael-conway]

I've pushed this to the snapshot for testing. So far so good.

[ghost]

Great! I'll wait for a stable release candidate then.