[GitHub] activemq-artemis pull request #1827: Improve paged message acknowledge

[shoukunhuai]

Github user shoukunhuai commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1827#discussion_r165284853
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/MessageReference.java
 ---
@@ -38,6 +38,10 @@ public static MessageReference createReference(Message 
encode, final Queue queue
 
Message getMessage();
 
+   default long getMessageID() {
--- End diff --

Thanks, will change it later


---

[GitHub] activemq-artemis pull request #1827: Improve paged message acknowledge

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1827#discussion_r165282547
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/MessageReference.java
 ---
@@ -38,6 +38,10 @@ public static MessageReference createReference(Message 
encode, final Queue queue
 
Message getMessage();
 
+   default long getMessageID() {
--- End diff --

-1 on hot path default methods on hot path aren't so good, suggest either 
leaving the code as was in ServerConsumerImpl with the 
getMessage().getMessageId or simply not make it default, and update other 
impl's within the code base.

nudge @franz1981 


---

[GitHub] activemq-artemis pull request #1827: Improve paged message acknowledge

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1827#discussion_r165282202
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/RefsOperation.java
 ---
@@ -152,14 +153,14 @@ protected void rollbackRedelivery(Transaction tx, 
MessageReference ref, long tim
@Override
public void afterCommit(final Transaction tx) {
   for (MessageReference ref : refsToAck) {
- synchronized (ref.getQueue()) {
--- End diff --

if this is removed should the same syn block on the queue in rollback be 
removed, im not 100% if this is safe though or not, is there a concurrent test 
case at all around this if not, could one be easily added?


---

[GitHub] activemq-artemis pull request #1827: Improve paged message acknowledge

[shoukunhuai]

Github user shoukunhuai commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1827#discussion_r165050956
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/RefsOperation.java
 ---
@@ -152,14 +153,14 @@ protected void rollbackRedelivery(Transaction tx, 
MessageReference ref, long tim
@Override
public void afterCommit(final Transaction tx) {
   for (MessageReference ref : refsToAck) {
- synchronized (ref.getQueue()) {
-queue.postAcknowledge(ref);
- }
+ queue.postAcknowledge(ref);
   }
 
   if (pagedMessagesToPostACK != null) {
  for (MessageReference refmsg : pagedMessagesToPostACK) {
-decrementRefCount(refmsg);
--- End diff --

@cshannon thanks.


---

[GitHub] activemq-artemis pull request #1827: Improve paged message acknowledge

[cshannon]

Github user cshannon commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/1827#discussion_r165035847
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/RefsOperation.java
 ---
@@ -152,14 +153,14 @@ protected void rollbackRedelivery(Transaction tx, 
MessageReference ref, long tim
@Override
public void afterCommit(final Transaction tx) {
   for (MessageReference ref : refsToAck) {
- synchronized (ref.getQueue()) {
-queue.postAcknowledge(ref);
- }
+ queue.postAcknowledge(ref);
   }
 
   if (pagedMessagesToPostACK != null) {
  for (MessageReference refmsg : pagedMessagesToPostACK) {
-decrementRefCount(refmsg);
--- End diff --

I agree that right now it doesn't seem to be of much use except for large 
messages.  However if this gets taken out I would be a little concerned about 
introducing a future bug if other message types end up relying on this in the 
future.


---

[GitHub] activemq-artemis pull request #1827: Improve paged message acknowledge

[shoukunhuai]

GitHub user shoukunhuai opened a pull request:

https://github.com/apache/activemq-artemis/pull/1827

Improve paged message acknowledge



Cache `messageID`, `transactionID` and `isLargeMessage`
in PagedReference, so that when acknowledge, we do not have to
get PagedMessage which may be GCed and cause re-read entire page.

Also, in RefsOperation::afterCommit do not lock queue when do
postAcknowledge. I think this method is thread safe.

And last, decrement ref count after commit only if it is a paged large 
message.
Large message depends on this operation to delete file. Other implements 
does nothing in this method.

All those change is to avoid get PagedMessage when acknowledge.

Let me know if there is anything wrong.

@clebertsuconic would you please help review this PR? I will create jira 
issue when necessary.

Attached is thread dump when doing stress test, we found most of server 
thread is blocked on page operation.
We use Artemis 2.4.0, run 10 jmeter engine, for each engine started 100 
publisher and 50 subscriber.
We changed jmeter to use shared subscription, and blockOnAcknowledge is set 
true.


[jstack.txt](https://github.com/apache/activemq-artemis/files/1680132/jstack.txt)


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/shoukunhuai/activemq-artemis enhance-paged-ack

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/1827.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 #1827


commit 52d63a5802353966236f1f9eec85d577e1941a12
Author: huaishk 
Date:   2018-01-31T01:48:43Z

Improve paged message acknowledge

Cache `messageID`, `transactionID` and `isLargeMessage`
in PagedReference, so that when acknowledge, we do not have to
get PagedMessage which may be GCed and cause re-read entire page.

Also, in RefsOperation::afterCommit do not lock queue when do
postAcknowledge. I think this method is thread safe.




---