[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-602733268 I'm planning to add another commit to try save completely reading the `Page`, or I should just send another pr for this. @wy96f @clebertsuconic seems that I cannot assign reviewers, so I'm calling out directly ;) 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-602855737 Sure bud, there is no hurry and this change should be simpler enough to be rebased against your when you have done ;) 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-603100401 I've run the CI tests and this change seems to work fine :+1 Perf-wise is a nice improvement over the original version, although not perfect: ideally we should skip reading the Page at all, but now we save a lot of CPU time and heap space by saving useless message decoding :+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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-603143665 @clebertsuconic I need some help to understand the impact of this change I've made on `Page::delete`: : - before: was calling `Message::usageDown` on each message - now: it's calling `Message::usageDown` only for large messages I see that ddd8ed440226fa9099f894fa0dd5c1e03614b7da has introduced `usageDown` to be triggered on ANY message, was it correct?  It's ok if I'm calling it just for large messages? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-603280293 @clebertsuconic has confirmed that `Message::usageDown` is menat to be used just on large messages and it has no meaningful effects on non large messages: it means that I can left the logic as I've implemented in this PR. I will add some doc around `Message::usageUp/usageDown` to explain explicitely that to avoid future misuses. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-606850755 @clebertsuconic yep, there has been some new changes around that part recently, so tomorrow will take care of rebasing ;) 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676
PageCursorProviderImpl::cleanup can save decoding pages without large messages
URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-607084834
@clebertsuconic I see on `PagedMessageImpl` this check:
```java
public boolean isLargeMessage() {
return message instanceof ICoreMessage &&
((ICoreMessage)message).isLargeMessage();
}
```
and
```
@Override
public void encode(final ActiveMQBuffer buffer) {
buffer.writeLong(transactionID);
boolean isLargeMessage = isLargeMessage();
buffer.writeBoolean(isLargeMessage);
```
why we cannot modify the check/encode/decode to correctly write the
`isLargeMessage` prefix field?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-607329180 @clebertsuconic I've tried to come up with a test to cover AMQP large and standard messages: let me know if the way I create and write `AMQPLargeMessage` is wrong. While working on it I've found a test failure on the new test `AmqpPageTest::testLargeMessagePageWithNIO` while checking `Assert.assertEquals(largeMessages ? 1 : 0, pagedMessage.getMessage().getUsage());` I have pushed a change trying to fix this, but maybe the `assert` is just wrong for AMQP, while it's valid only for Core large messages. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-607356882 I've pushed the changed on `usage` count on https://github.com/apache/activemq-artemis/pull/3055 + some of the enhancements on the test I have here: so please don't merge this yet. I will rebase over master wehn https://github.com/apache/activemq-artemis/pull/3055 will be merged 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-607692028 I've rebased against master: the logic should be correct, but I would like to add some unit tests around the introduced logic too :) Let me know if the logic seems sound 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-609096425 I see that this one is a kind of major perf improvement on paging, but I still want to add some more tests on binary compatibility of PagedMessage decoding that I see we currently don't cover on tests 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages
franz1981 commented on issue #3044: ARTEMIS-2676 PageCursorProviderImpl::cleanup can save decoding pages without large messages URL: https://github.com/apache/activemq-artemis/pull/3044#issuecomment-612660737 I will try to add the missing tests in the next days 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: us...@infra.apache.org With regards, Apache Git Services
