You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by hirthwork <gi...@git.apache.org> on 2014/11/11 14:24:49 UTC
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
GitHub user hirthwork opened a pull request:
https://github.com/apache/httpcore/pull/7
Reduce synchronized scopes in IOSessionImpl
Currently IOSessionImpl has too many synchronized methods which results in poor performance on high load.
Changes consist of two commits:
First one removes synchronization from lastRead/Write/AccessTime variables access and makes this variables volatile.
Second one introduces double checks for status member-field which is already volatile. At the same time, synchronized (this) scope in IOSessionImpl.close() reduced to have double checks faster.
In order to avoid CancelledKeyException in .toString() member-function key cancellation is made in scope synchronized on this key.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hirthwork/httpcore 4.4.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/httpcore/pull/7.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #7
----
commit e81a8a4745e6c56797099210f2426f618bd00ca8
Author: Dmitry Potapov <dp...@yandex-team.ru>
Date: 2014-11-11T13:06:18Z
IOSessionImpl: use volatiles for time access functions instead of synchronized access
commit 8d8a8aa57470a9e32de9a9857216eb666c1cb005
Author: Dmitry Potapov <dp...@yandex-team.ru>
Date: 2014-11-11T13:16:48Z
IOSessionImpl: use double checks whenever it is possible to reduce synchronized scope
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by hirthwork <gi...@git.apache.org>.
Github user hirthwork commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62960585
f0f1a6693690774c1cb0 still have some issues (not related to the IOSessionImpl) with asynchronous connections closing, so let it be just a gist, but not the part of main repo.
BTW, are there any upcoming changes to trunk, which won't allow to propagate this commit to 5.0?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by hirthwork <gi...@git.apache.org>.
Github user hirthwork commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62673889
Pull request simplified. The only blocking operation is .close() (because of dup2 called by preClose0), so no double checks required. Just synchronized scope of .close() reduced to have quick check and set status and release monitor lock.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by hirthwork <gi...@git.apache.org>.
Github user hirthwork commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62743426
One weird thing concerning async channel close: there is no time guarantees concerning FIN packet sending. Close requests are being stored to the executor queue, so lag between last PSH and consequent FIN can be significant and have the same order as the SO_LINGER timeout. This is actual scenario when client drops connection without reading the socket buffer.
That's why I recommend to have aforementioned crutches somewhere in examples section, but not in the main source tree.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by hirthwork <gi...@git.apache.org>.
Github user hirthwork commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62699517
Yes, this is a part of performance improvement, but I don't believe you want to have these [crutches](https://gist.github.com/hirthwork/f0f1a6693690774c1cb0) in master. The idea is to move channel.close() to separate thread. Channel can be also closed by selector, so before closing channel is being registered in fake selector, so main IOReactor thread won't be blocked.
Using volatiles for lastRead/Write/AccessTime is the generic improvement which will improve performance of current implementation.
Using atomic for status doesn't look like a fair tradeoff. You will improve performance of .close() function a bit, but other functions will be forced to call status.get() inside the lock, so overall performance could degrade. close() is called once per session lifetime while event mask operations can be called several times (hundreds of times for proxies like NHttpReverseProxy from examples).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-63046705
> BTW, are there any upcoming changes to trunk, which won't allow to propagate this commit to 5.0?
I intend to revise (and likely rewrite) most of the NIO reactor code for 5.0. In particular I would like to simplify IOSessionImpl and try to reduce footprint of code that requires synchronization. I also need to rethink the way SSL is done. In fact, this might be the right moment to consider using a third party NIO framework instead of our custom code. But let's discuss it on the dev list.
Oleg
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62780306
OK. Changes committed to SVN 4.4.x branch as
http://svn.apache.org/r1638960
http://svn.apache.org/r1638961
I am fairly certain that AtomicInteger#get is as efficient as reading from a volatile int, and therefore should not cause any difference in terms of performance, but I am not gonna argue.
Oleg
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62784237
As far as f0f1a6693690774c1cb0 and similar useful but out of scope code is concerned, we have the so called 'contrib' area in the SVN repository [1] primarily intended for such contributions. However, in all honesty, these days one might be much better off keeping his/her code in GitHub rather than some obscure branch of the ASF SVN repository no one likely to care about. Having said all that you are very welcome to contribute that code if you like.
Oleg
[1] http://svn.apache.org/repos/asf/httpcomponents/contrib/httpcore/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by hirthwork <gi...@git.apache.org>.
Github user hirthwork commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-63060374
As for me, current NIO implementation works pretty good and integration between HttpCore/NIO/Client and AsyncClient allows to have same helper functions for requests processing. Introducing third party NIO will break integration between HttpCore and AsyncClient, which is not desirable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by hirthwork <gi...@git.apache.org>.
Github user hirthwork closed the pull request at:
https://github.com/apache/httpcore/pull/7
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62690756
Hi Dmitry
I understand the rationale behind these changes but do they really result in a tangible performance improvement? You see, generally IOSession should mostly be interacted with by one IO dispatch thread and therefore there should almost no thread contention to start with (besides IO interest operations).
I also would replace this bit in #close() method with AtomicInteger#compareAndSet
```java
synchronized (this) {
if (this.status == CLOSED) {
return;
}
this.status = CLOSED;
}
```
Oleg
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcore pull request: Reduce synchronized scopes in IOSessionImpl
Posted by hirthwork <gi...@git.apache.org>.
Github user hirthwork commented on the pull request:
https://github.com/apache/httpcore/pull/7#issuecomment-62780779
Thanks a lot
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org