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 2022/10/18 03:35:40 UTC
[GitHub] [activemq-artemis] clebertsuconic opened a new pull request, #4260: ARTEMIS-4056 Paging Management optimizations
clebertsuconic opened a new pull request, #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260
- optimize startup time on paging (check-depage on startup)
- otpimize getNextPage() on complete pages
- optimize getFirstMessage() and paging. (avoid iterator usage)
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4260: ARTEMIS-4056 Paging Management optimizations
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260#discussion_r998179693
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java:
##########
@@ -883,17 +883,12 @@ protected Map<String, Object>[] getFirstMessage() throws Exception {
clearIO();
try {
List<Map<String, Object>> messages = new ArrayList<>();
- queue.flushExecutor();
final int attributeSizeLimit = addressSettingsRepository.getMatch(address).getManagementMessageAttributeSizeLimit();
- try (LinkedListIterator<MessageReference> iterator = queue.browserIterator()) {
- // returns just the first, as it's the first only
- if (iterator.hasNext()) {
- MessageReference ref = iterator.next();
- Message message = ref.getMessage();
- messages.add(message.toMap(attributeSizeLimit));
- }
- return messages.toArray(new Map[1]);
+ MessageReference firstMessage = queue.peekFirstMessage();
+ if (firstMessage != null) {
+ messages.add(firstMessage.getMessage().toMap(attributeSizeLimit));
}
+ return messages.toArray(new Map[1]);
Review Comment:
the idea is to avoid the iterator() for the first message. This might cause an outage if the system is under stress already.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4260: ARTEMIS-4056 Paging Management optimizations
Posted by GitBox <gi...@apache.org>.
gemmellr commented on code in PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260#discussion_r998150771
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -832,13 +832,14 @@ public boolean isPersistent() {
@Override
public void processReload() throws Exception {
if (recoveredACK != null) {
- logger.trace("********** processing reload!!!!!!!");
+ logger.debug("processing reload queue name={} with id={}", queue != null ? this.queue.getName() : "N/A", this.cursorId);
Review Comment:
The cursorId value is a long, so you should now gate the logger to avoid boxing. The "this" qualifiers dont seem needed.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java:
##########
@@ -883,17 +883,12 @@ protected Map<String, Object>[] getFirstMessage() throws Exception {
clearIO();
try {
List<Map<String, Object>> messages = new ArrayList<>();
- queue.flushExecutor();
final int attributeSizeLimit = addressSettingsRepository.getMatch(address).getManagementMessageAttributeSizeLimit();
- try (LinkedListIterator<MessageReference> iterator = queue.browserIterator()) {
- // returns just the first, as it's the first only
- if (iterator.hasNext()) {
- MessageReference ref = iterator.next();
- Message message = ref.getMessage();
- messages.add(message.toMap(attributeSizeLimit));
- }
- return messages.toArray(new Map[1]);
+ MessageReference firstMessage = queue.peekFirstMessage();
+ if (firstMessage != null) {
+ messages.add(firstMessage.getMessage().toMap(attributeSizeLimit));
}
+ return messages.toArray(new Map[1]);
Review Comment:
If the idea is to optimise this overall getFirstMessage() method, then there still looks to be room for improvement here. It creates an ArrayList of Map, then afterwards figures out whether it will put 0 or 1 things in it, causing creation of a new array of capacity 10 if there is, and then converts the resulting ArrayList of 0/1 items into a new created array of size 1.
Seems like it could just omit the ArrayList and create the final array, then if the map value exists add it, then return it. Or if allowed by the API and its uses, even just only create the array once its known there is something to put in it, and return null otherwise.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1460,23 +1473,47 @@ private PagedReference moveNext() {
private PagedReference internalGetNext() {
for (;;) {
+ if (logger.isDebugEnabled() && currentPageIterator == null) {
+ logger.debug("Page Cursor {} has no pageIterator", cursorId, new Exception("trace"));
Review Comment:
'tracing exception stacktrace' at debug seems odd.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1278,11 +1286,16 @@ private class CursorIterator implements PageIterator {
private LinkedListIterator<PagedMessage> currentPageIterator;
private void initPage(long page) {
+ logger.debug("initPage {}", page);
Review Comment:
should be gated
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1460,23 +1473,47 @@ private PagedReference moveNext() {
private PagedReference internalGetNext() {
for (;;) {
+ if (logger.isDebugEnabled() && currentPageIterator == null) {
Review Comment:
Is the null value unlikely to happen? Seems like it since it will presumably NPE below if so. (EDIT: so do we even need the logging?)
If its the rarer event, I'd put its check first. I'd probably just make it 2 ifs.
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1460,23 +1473,47 @@ private PagedReference moveNext() {
private PagedReference internalGetNext() {
for (;;) {
+ if (logger.isDebugEnabled() && currentPageIterator == null) {
+ logger.debug("Page Cursor {} has no pageIterator", cursorId, new Exception("trace"));
+ }
PagedMessage message = currentPageIterator.hasNext() ? currentPageIterator.next() : null;
logger.trace("CursorIterator::internalGetNext:: new reference {}", message);
if (message != null) {
return cursorProvider.newReference(message, PageSubscriptionImpl.this);
}
- if (currentPage.getPageId() < pageStore.getCurrentWritingPage()) {
+ if (logger.isTraceEnabled()) {
+ logger.trace("Current page {}", currentPage != null ? currentPage.getPageId() : null);
+ }
+ long nextPage = getNextPage();
+ if (logger.isTraceEnabled()) {
+ logger.trace("next page {}", nextPage);
+ }
+ if (nextPage >= 0) {
if (logger.isTraceEnabled()) {
- logger.trace("CursorIterator::internalGetNext:: moving to currentPage {}", currentPage.getPageId() + 1);
+ logger.trace("CursorIterator::internalGetNext:: moving to currentPage {}", nextPage);
}
- initPage(currentPage.getPageId() + 1);
+ initPage(nextPage);
} else {
return null;
}
}
}
+ private long getNextPage() {
+ long page = currentPage.getPageId() + 1;
+
+ while (page <= pageStore.getCurrentWritingPage()) {
+ PageCursorInfo info = locatePageInfo(page);
+ if (info == null || info.getCompleteInfo() == null) {
+ return page;
+ }
+ logger.debug("Subscription {} named {} moving faster from page {} to next", cursorId, queue.getName(), page);
Review Comment:
Triple arg, and long values...add gate
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1278,11 +1286,16 @@ private class CursorIterator implements PageIterator {
private LinkedListIterator<PagedMessage> currentPageIterator;
private void initPage(long page) {
+ logger.debug("initPage {}", page);
try {
if (currentPage != null) {
+ if (logger.isTraceEnabled()) {
+ logger.trace("usage down {} on subscription {}", currentPage.getPageId(), cursorId);
+ }
currentPage.usageDown();
}
if (currentPageIterator != null) {
+ logger.trace("closing pageIterator on {}", cursorId);
Review Comment:
should be gated
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -3164,6 +3174,7 @@ private void checkDepage() {
return;
}
if (pageIterator != null && pageSubscription.isPaging()) {
+ logger.debug("CheckDepage on queue name {}, id={}", this.name, this.id);
Review Comment:
The "this." qualifiers seem unnecessary.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4260: ARTEMIS-4056 Paging Management optimizations
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260#discussion_r998288788
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java:
##########
@@ -883,17 +883,12 @@ protected Map<String, Object>[] getFirstMessage() throws Exception {
clearIO();
try {
List<Map<String, Object>> messages = new ArrayList<>();
- queue.flushExecutor();
final int attributeSizeLimit = addressSettingsRepository.getMatch(address).getManagementMessageAttributeSizeLimit();
- try (LinkedListIterator<MessageReference> iterator = queue.browserIterator()) {
- // returns just the first, as it's the first only
- if (iterator.hasNext()) {
- MessageReference ref = iterator.next();
- Message message = ref.getMessage();
- messages.add(message.toMap(attributeSizeLimit));
- }
- return messages.toArray(new Map[1]);
+ MessageReference firstMessage = queue.peekFirstMessage();
+ if (firstMessage != null) {
+ messages.add(firstMessage.getMessage().toMap(attributeSizeLimit));
}
+ return messages.toArray(new Map[1]);
Review Comment:
it makes sense... after reading what it does..
I was just getting rid of the iterator() which caused an outage on an user I was helping out.
But I will improve that part as well.. 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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4260: ARTEMIS-4056 Paging Management optimizations
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260#discussion_r998308010
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java:
##########
@@ -1460,23 +1473,47 @@ private PagedReference moveNext() {
private PagedReference internalGetNext() {
for (;;) {
+ if (logger.isDebugEnabled() && currentPageIterator == null) {
Review Comment:
I replaced it by an assertion...
I had a System.out.println() that I decided to keep it as a logger.....(I had an issue as I was developing).
Reading it back after done now, an assert would make more sense.
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #4260: ARTEMIS-4056 Paging Management optimizations
Posted by GitBox <gi...@apache.org>.
gemmellr commented on PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260#issuecomment-1282504694
LGTM
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a diff in pull request #4260: ARTEMIS-4056 Paging Management optimizations
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on code in PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260#discussion_r998304949
##########
artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/QueueImpl.java:
##########
@@ -3164,6 +3174,7 @@ private void checkDepage() {
return;
}
if (pageIterator != null && pageSubscription.isPaging()) {
+ logger.debug("CheckDepage on queue name {}, id={}", this.name, this.id);
Review Comment:
@gemmellr I actually like the this quite a lot.. makes it explicitly and where it's coming from when you read it.
I guess it will be personal taste here?
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic merged pull request #4260: ARTEMIS-4056 Paging Management optimizations
Posted by GitBox <gi...@apache.org>.
clebertsuconic merged PR #4260:
URL: https://github.com/apache/activemq-artemis/pull/4260
--
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.
To unsubscribe, e-mail: gitbox-unsubscribe@activemq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org