You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/02/21 20:01:54 UTC
[GitHub] [activemq-artemis] franz1981 opened a new pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
franz1981 opened a new pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462
https://issues.apache.org/jira/browse/ARTEMIS-3133
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-784180696
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#discussion_r581017444
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/ServerSessionPacketHandler.java
##########
@@ -921,7 +979,11 @@ private void doConfirmAndResponse(final Packet confirmPacket,
}
if (response != null) {
- channel.send(response);
+ try {
+ channel.send(response);
Review comment:
is this blocking or async, i ask, because if its async you need to be careful, putting back the object into the pool, as it may not be finished with.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-784028355
I've addressed all the test failures: I'm going to try a run on the CI to see if it's fine :)
Porbably the client session packet handler could benefit from a similar improvement ;)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-784180696
@clebertsuconic
I've gotten a test failure on `org.apache.activemq.artemis.tests.integration.cluster.distribution.ClusteredGroupingTes`:
```
Exception in thread "activemq-buffer-timeout" org.apache.activemq.artemis.api.core.ActiveMQInterruptedException: java.lang.InterruptedException
at org.apache.activemq.artemis.core.io.buffer.TimedBuffer$CheckTimer.run(TimedBuffer.java:484)
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.InterruptedException
at java.util.concurrent.locks.AbstractQueuedSynchronizer.doAcquireSharedInterruptibly(AbstractQueuedSynchronizer.java:998)
at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireSharedInterruptibly(AbstractQueuedSynchronizer.java:1304)
at java.util.concurrent.Semaphore.acquire(Semaphore.java:312)
at org.apache.activemq.artemis.core.io.buffer.TimedBuffer$CheckTimer.run(TimedBuffer.java:478)
... 1 more
java.lang.NullPointerException
at org.apache.activemq.artemis.tests.integration.cluster.distribution.ClusterTestBase.startServers(ClusterTestBase.java:1931)
at org.apache.activemq.artemis.tests.integration.cluster.distribution.ClusteredGroupingTest.cycleServer(ClusteredGroupingTest.java:784)
at org.apache.activemq.artemis.tests.integration.cluster.distribution.ClusteredGroupingTest.access$300(ClusteredGroupingTest.java:57)
at org.apache.activemq.artemis.tests.integration.cluster.distribution.ClusteredGroupingTest$6.run(ClusteredGroupingTest.java:693)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
at org.apache.activemq.artemis.utils.ActiveMQThreadFactory$1.run(ActiveMQThreadFactory.java:118)
```
It doesn't seems related to this change really, but need to check if is reproducible on my box too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-783984909
I'm just getting a failure on `org.apache.activemq.artemis.tests.integration.jms.cluster.TopicClusterPageStoreSizeTest.testPageStoreSizeWithClusteredDurableSub` that I'm addressing
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-783960692
@clebertsuconic I just noticed the reason :+1: thanks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#discussion_r581029960
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/ServerSessionPacketHandler.java
##########
@@ -921,7 +979,11 @@ private void doConfirmAndResponse(final Packet confirmPacket,
}
if (response != null) {
- channel.send(response);
+ try {
+ channel.send(response);
Review comment:
it's completely sync and won't retain the `Packet` unless the conf window size is != -1, where it can use a `resendCache` to store it.
In this case the caching won't happen, see https://github.com/apache/activemq-artemis/pull/3462/files#diff-c58d25d4978ebff7d03f1e5858b8b3344b01b3f2376e5ff194483d85c7b36a09R200-R208
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] michaelpearce-gain commented on a change in pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
michaelpearce-gain commented on a change in pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#discussion_r581017444
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/ServerSessionPacketHandler.java
##########
@@ -921,7 +979,11 @@ private void doConfirmAndResponse(final Packet confirmPacket,
}
if (response != null) {
- channel.send(response);
+ try {
+ channel.send(response);
Review comment:
is this blocking or async the handling of the packet within the channel, i ask, because if its async you need to be careful, putting back the object into the pool, as it may not be finished with, and i thought within netty bits like that are handed off, i could be wrong...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-783960692
@clebertsuconic I just noticed the reason :+1: thanks
I see that we cannot recycle null packets instances that easily because of `sendResponse` that can send responses back on `storageManager.afterCompleteOperations` using a different thread :(
I'm preparing something to handle this case too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#discussion_r581029960
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/ServerSessionPacketHandler.java
##########
@@ -921,7 +979,11 @@ private void doConfirmAndResponse(final Packet confirmPacket,
}
if (response != null) {
- channel.send(response);
+ try {
+ channel.send(response);
Review comment:
it's completely sync and won't retain the `Packet` unless the conf window size is != -1, where it can use a `resendCache` to retaining it.
In this case the caching won't happen, see https://github.com/apache/activemq-artemis/pull/3462/files#diff-c58d25d4978ebff7d03f1e5858b8b3344b01b3f2376e5ff194483d85c7b36a09R200-R208
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-783797533
I had a few test failures when I rebased this and run the whole test suite.
Did you run a full test on this?
Test Name | Duration | Age
-- | -- | --
org.apache.activemq.artemis.tests.integration.cluster.distribution.LargeMessageRedistributionTest.testRedistributionLargeMessageDirCleanup | 33 sec | 1
org.apache.activemq.artemis.tests.integration.federation.FederatedQueueTest.testFederatedQueueBiDirectionalDownstream | 2.5 sec | 1
org.apache.activemq.artemis.tests.integration.jms.cluster.TopicClusterPageStoreSizeTest.testPageStoreSizeWithClusteredDurableSub | 61 ms | 1
org.apache.activemq.artemis.tests.integration.persistence.SyncSendTest.testSendConsumeAudoACK[storage=libaio, protocol=core] | 0.48 sec | 1
org.apache.activemq.artemis.tests.integration.persistence.SyncSendTest.testSendConsumeAudoACK[storage=libaio, protocol=openwire] | 0.47 sec | 1
org.apache.activemq.artemis.tests.integration.persistence.SyncSendTest.testSendConsumeAudoACK[storage=libaio, protocol=amqp] | 0.29 sec | 1
org.apache.activemq.artemis.tests.integration.persistence.SyncSendTest.testSendConsumeAudoACK[storage=nio, protocol=core] | 0.34 sec | 1
org.apache.activemq.artemis.tests.integration.persistence.SyncSendTest.testSendConsumeAudoACK[storage=nio, protocol=openwire] | 0.28 sec | 1
org.apache.activemq.artemis.tests.integration.persistence.SyncSendTest.testSendConsumeAudoACK[storage=nio, protocol=amqp] | 0.29 sec | 1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 edited a comment on pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#issuecomment-783960692
@clebertsuconic I just noticed the reason :+1: thanks
I see that we cannot recycle null packets instances that easily because of `sendResponse` that can send responses back on `storageManager.afterCompleteOperations` using a different thread :(
I'm preparing something to handle this case as a separate commit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3462: ARTEMIS-3133 Reduce memory usage for Null an Ping packets
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3462:
URL: https://github.com/apache/activemq-artemis/pull/3462#discussion_r581029960
##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/ServerSessionPacketHandler.java
##########
@@ -921,7 +979,11 @@ private void doConfirmAndResponse(final Packet confirmPacket,
}
if (response != null) {
- channel.send(response);
+ try {
+ channel.send(response);
Review comment:
it's completely sync and won't retain the `Packet` unless the conf window size is != -1, where it can use a `resendCache` to store it (and a `responseAsyncCache` too).
In this case the caching won't happen, see https://github.com/apache/activemq-artemis/pull/3462/files#diff-c58d25d4978ebff7d03f1e5858b8b3344b01b3f2376e5ff194483d85c7b36a09R200-R208
and
https://github.com/apache/activemq-artemis/blob/master/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ChannelImpl.java#L390-L406
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org