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 2020/01/21 22:29:33 UTC
[GitHub] [activemq-artemis] franz1981 opened a new pull request #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
franz1981 opened a new pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950
I've opened this for discussion, so PLEASE DON'T MERGE IT YET.
I've yet to include the same optimization for other properties (ie `getDuplicateProperty` and `getLastValueProperty`) and for the `AMQPMessage`.
I will need some help from @gemmellr and @tabish121 , because I see that implementing an `AMQPMessage::searchProperty(SimpleString key)` that won't force encoding (if not already happened) of the whole message isn't trivial, due to my ignorance of the protocol :(
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577046495
@clebertsuconic Sadly seems that while exchanging messages, we trigger the full decode anyway, due to other properties accessed eg `CoreMessage:.getRoutingType`
But, good news about the change already done:
that's from *ARTEMIS 2.11*
And the same heap configuration ie `-XX:+UseG1GC -Xms4G -Xmx4G -XX:+AlwaysPreTouch` for *this PR*:
The difference in max pause time is just too much (*7 sec vs 150 ms*!!!!!) to compare them :)
There are other metrics, but the point is that we won't produce tons of garbage to decode messages if the `ScheduledDeliveryTime` property isn't set and should cover the most of the cases.
The previous results are obtained by loading a journal created by running 30 pairs of Core JMS producers/consumers sending persistent byte messages of 10 bytes size until the broker start to page, with slow consumers (ie performing `Thread::sleep(200)` while consuming messages): this has filled the message journal with a mix of unacked/acked small messages that would stress the heap/GC part of the loading activity instead of the disk.
Indeed the whole `data` folder is just `2.5 GB`.
I would try with different sizes too, to put under stress the new search property method, that is being benchmarked separately to make it as fast as possible.
I strongly encourage any user that make heavy use of the journal and has stress tests while loading journal to try this (@michaelandrepearce @wy96f ?) being aware that ATM it just improve this for Core protocol, but with some help, will get the same for AMQP as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369697534
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
Re your comments
> You're still parsing the key at its exact position.
Nope: to compare `SimpleString` and `ByteBuf` is using another optimized method I've implemented some time ago that allow to compare them very quickly directly ie https://github.com/apache/activemq-artemis/blob/0293d8057441e6bfeb9e6533774862ffc86d74eb/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ByteUtil.java#L346-L360
> in Java every time you do buffer.getInt(index) you will create a few bytes in memory.
That's the signature of the Netty method: https://netty.io/4.0/api/io/netty/buffer/ByteBuf.html#getInt-int-
it would return a primitive `int`: it's not allocating anything. I've taken care to design that method to be garbage-free :+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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369658086
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
what I mean is.. if searching the whole buffer like a string chain is faster.. just go for it.. don't worry about values as keys.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2950: ARTEMIS-2604
Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-580207415
@michaelandrepearce @clebertsuconic I've added some tests and this is ready to get a full review.
After addressing all the review comments I'll squash the changes into one. The CI is already in a good shape on this :+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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577260402
>"the ideal thing would be to always place ScheduledDeliveryTime " in a fixed position (as first property?)
@franz1981 :
the ideal thing would be to use a fixed value within the protocol, like AMQP does. requiring a fixed position would pretty much be redefining the protocol, it would incur in versioning anyways.
Using a fixed property would in fact require you to do versioning on the protocol, to make sure you still parse the whole buffer for older clients.. so it's not feasible without breaking compatibility, and if you break it, it would be best to do the right thing.
I would say it's a no. .since AMQP takes care of it. we could instead invest to make AMQP faster than core.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577260402
"the ideal thing would be to always place ScheduledDeliveryTime"
the ideal thing would be to use a fixed value within the protocol, like what AMQP guys are doing. it would pretty much be redefining the protocol, it would incur in versioning anyways.
Using a fixed property would in fact require you to do versioning on the protocol, to make sure you still parse the whole buffer for older clients.. so it's not feasible without breaking compatibility, and if you break it, it would be best to do the right thing.
I would say it's a no. .since AMQP takes care of it. we could instead invest to make AMQP faster than core.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577046495
@clebertsuconic Sadly seems that while exchanging messages, we trigger the full decode anyway, due to other properties accessed eg `CoreMessage:.getRoutingType`
But, good news about the change already done:
that's from *ARTEMIS 2.11*
And the same heap configuration ie `-XX:+UseG1GC -Xms4G -Xmx4G -XX:+AlwaysPreTouch` for *this PR*:
The difference in pause time is just too much to compare them :)
There are other metrics, but the point is that we won't produce tons of garbage to decode messages if the `ScheduledDeliveryTime` property isn't set and should cover the most of the cases.
The previous results are obtained by loading a journal created by running 30 pairs of Core JMS producers/consumers sending persistent byte messages of 10 bytes size until the broker start to page, with slow consumers (ie performing `Thread::sleep(200)` while consuming messages): this has filled the message journal with a mix of unacked/acked small messages that would stress the heap/GC part of the loading activity instead of the disk.
Indeed the whole `data` folder is just `2.5 GB`.
I would try with different sizes too, to put under stress the new search property method, that is being benchmarked separately to make it as fast as possible.
I strongly encourage any user that make heavy use of the journal and has stress tests while loading journal to try this, being aware that ATM it just improve this for Core protocol, but with some help, will get the same for AMQP as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369656249
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
I will change it for sure :+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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577046495
@clebertsuconic Sadly seems that while exchanging messages, we trigger the full decode anyway, due to other properties accessed eg `CoreMessage:.getRoutingType`
But, good news about the change already done:
that's from *ARTEMIS 2.11*
And the same heap configuration ie `-XX:+UseG1GC -Xms4G -Xmx4G -XX:+AlwaysPreTouch` for *this PR*:
The difference in max pause time is just too much (*7 sec vs 150 ms* to compare them :)
There are other metrics, but the point is that we won't produce tons of garbage to decode messages if the `ScheduledDeliveryTime` property isn't set and should cover the most of the cases.
The previous results are obtained by loading a journal created by running 30 pairs of Core JMS producers/consumers sending persistent byte messages of 10 bytes size until the broker start to page, with slow consumers (ie performing `Thread::sleep(200)` while consuming messages): this has filled the message journal with a mix of unacked/acked small messages that would stress the heap/GC part of the loading activity instead of the disk.
Indeed the whole `data` folder is just `2.5 GB`.
I would try with different sizes too, to put under stress the new search property method, that is being benchmarked separately to make it as fast as possible.
I strongly encourage any user that make heavy use of the journal and has stress tests while loading journal to try this (@michaelandrepearce @wy96f ?) being aware that ATM it just improve this for Core protocol, but with some help, will get the same for AMQP as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
Is about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11i have to raise it to 6 GB to make the loading being quick as 3 GB.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2950: ARTEMIS-2604
Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577261784
To make amqp to be faster then core we should start dropping boxed values as viable decoded types and just use primitives: that would help a lot
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369658086
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
what I mean is.. if parsing the whole buffer is faster.. just go for it.. don't worry about values as keys.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2950: ARTEMIS-2604
Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577388373
@michaelandrepearce I've yet to add some tests, change some name and cleanup some APIs by addressing some of the advices from @clebertsuconic , but feel free to take a look/review so I will address altogether: from the pov of journal loading (and ha shared store) I see that when the disk is fast enough this could be really a game changer together with the other optimizations already merged on master, if compared to 2.11 :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577262343
>To make amqp to be faster then core we should start dropping boxed values as viable decoded types and just use primitives: that would help a lot
That requires a new codec, I think it's on the way.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369697534
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
Re your comments
> You're still parsing the key at its exact position.
I'm comparing, but I'm not parsing (there is no decoding involved).
In additon, to compare `SimpleString` and `ByteBuf` is using another optimized method I've implemented some time ago that allow to compare them very quickly directly ie https://github.com/apache/activemq-artemis/blob/0293d8057441e6bfeb9e6533774862ffc86d74eb/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ByteUtil.java#L346-L360
> in Java every time you do buffer.getInt(index) you will create a few bytes in memory.
That's the signature of the Netty method: https://netty.io/4.0/api/io/netty/buffer/ByteBuf.html#getInt-int-
it would return a primitive `int`: it's not allocating anything. I've taken care to design that method to be garbage-free :+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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369658086
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
what I mean is.. if searching the whole buffer is like a string chain is faster.. just go for it.. don't worry about values as keys.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-580207415
@michaelandrepearce @clebertsuconic I've added some tests and this is ready to get a full review.
After addressing all the review comments I'll squash the changes into one. The CI is already in a good shape on this :+1:
There is just one thing that worries me about these optimizations and is related to theirs fragility: how to ensure that future changes in the journal loading process won't trigger by accident the encoding of the message? That was a major cause of GC generation
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
It seems about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11i have to raise it to 6 GB to make the loading being quick as 3 GB.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
Is about twice as faster assuming a very fast disk :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
It seems about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11 i have to raise it to 6 GB to make the loading being quick as 3 GB.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577260402
>"the ideal thing would be to always place ScheduledDeliveryTime " in a fixed position (as first property?)
@franz1981 :
the ideal thing would be to use a fixed value within the protocol, like what AMQP guys are doing. it would pretty much be redefining the protocol, it would incur in versioning anyways.
Using a fixed property would in fact require you to do versioning on the protocol, to make sure you still parse the whole buffer for older clients.. so it's not feasible without breaking compatibility, and if you break it, it would be best to do the right thing.
I would say it's a no. .since AMQP takes care of it. we could instead invest to make AMQP faster than core.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] asfgit closed pull request #2950: ARTEMIS-2604
Optimizations on journal loading
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #2950: ARTEMIS-2604 Optimizations on journal loading
URL: https://github.com/apache/activemq-artemis/pull/2950
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
Is about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of the algorithm of loading, in reality, it uses much less memory then before and then, hit much less full GCs.
In my tests, with. 2.5 GB and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11i have to raise it to 6 GB to make the loading being quick as 3 GB.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577260402
>> "the ideal thing would be to always place ScheduledDeliveryTime"
the ideal thing would be to use a fixed value within the protocol, like what AMQP guys are doing. it would pretty much be redefining the protocol, it would incur in versioning anyways.
Using a fixed property would in fact require you to do versioning on the protocol, to make sure you still parse the whole buffer for older clients.. so it's not feasible without breaking compatibility, and if you break it, it would be best to do the right thing.
I would say it's a no. .since AMQP takes care of it. we could instead invest to make AMQP faster than core.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
It seems about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11 i have to raise it to 6 GB to make the loading quick as this pr with 3 GB.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
It seems about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB if journal and a 4 GB broker, with this PR I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11 i have to raise it to 6 GB to make the loading quick as this pr with 3 GB. And I still need to take care of Long boxing, probably in a follow up pr ;)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577260402
>"the ideal thing would be to always place ScheduledDeliveryTime"
@franz1981 :
the ideal thing would be to use a fixed value within the protocol, like what AMQP guys are doing. it would pretty much be redefining the protocol, it would incur in versioning anyways.
Using a fixed property would in fact require you to do versioning on the protocol, to make sure you still parse the whole buffer for older clients.. so it's not feasible without breaking compatibility, and if you break it, it would be best to do the right thing.
I would say it's a no. .since AMQP takes care of it. we could instead invest to make AMQP faster than core.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369697534
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
Re your comments
> You're still parsing the key at its exact position.
Nope: to compare `SimpleString` and `ByteBuf` is using another optimized method I've implemented some time ago that allow to compare them very quickly directly ie https://github.com/apache/activemq-artemis/blob/0293d8057441e6bfeb9e6533774862ffc86d74eb/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ByteUtil.java#L345
> in Java every time you do buffer.getInt(index) you will create a few bytes in memory.
That's the signature of the Netty method: https://netty.io/4.0/api/io/netty/buffer/ByteBuf.html#getInt-int-
it would return a primitive `int`: it's not allocating anything. I've taken care to design that method to be garbage-free :+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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] michaelandrepearce commented on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
michaelandrepearce commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577386732
just adding the DNMY label, as you said this isn't to be merged in first comment, just so its clearer when looking over all PR's. (I'm playing catchup from a nice long holiday :) )
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369654970
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
I have added a comment about this and the risk is to find a value that matches the property we are searching for. The other reason is that we know how many bytes to skip by using the decoded info and would help to reach quicker what we need without comparing every single byte (including values)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369697534
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
Re your comments
> You're still parsing the key at its exact position.
It's comparing, but it's not parsing (there is no decoding involved).
In additon, to compare `SimpleString` and `ByteBuf` is using another optimized method I've implemented some time ago that allow to compare them very quickly directly ie https://github.com/apache/activemq-artemis/blob/0293d8057441e6bfeb9e6533774862ffc86d74eb/artemis-commons/src/main/java/org/apache/activemq/artemis/utils/ByteUtil.java#L346-L360
> in Java every time you do buffer.getInt(index) you will create a few bytes in memory.
That's the signature of the Netty method: https://netty.io/4.0/api/io/netty/buffer/ByteBuf.html#getInt-int-
it would return a primitive `int`: it's not allocating anything. I've taken care to design that method to be garbage-free :+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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2950: ARTEMIS-2604
Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577050668
@clebertsuconic The downside of this optimization is that it still need to read the whole part of buffer with the properties in the best case: the ideal thing would be to always place `ScheduledDeliveryTime` in a fixed position (as first property?)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577046495
@clebertsuconic Sadly seems that while exchanging messages, we trigger the full decode anyway, due to other properties accessed eg `CoreMessage:.getRoutingType`
But, good news about the change already done:
that's from *ARTEMIS 2.11*
And the same heap configuration ie `-XX:+UseG1GC -Xms4G -Xmx4G -XX:+AlwaysPreTouch` for *this PR*:
The difference in pause time is just too much to compare them :)
There are other metrics, but the point is that we won't produce tons of garbage to decode messages if the `ScheduledDeliveryTime` property isn't set and should cover the most of the cases.
The previous results are obtained by loading a journal created by running 30 pairs of Core JMS producers/consumers sending persistent byte messages of 10 bytes size until the broker start to page, with slow consumers (ie performing `Thread::sleep(200)` while consuming messages): this has filled the message journal with a mix of unacked/acked small messages that would stress the heap/GC part of the loading activity instead of the disk.
Indeed the whole `data` folder is just `2.5 GB`.
I would try with different sizes too, to put under stress the new search property method, that is being benchmarked separately to make it as fast as possible.
I strongly encourage any user that make heavy use of the journal and has stress tests while loading journal to try this (@michaelandrepearce @wy96f ?) being aware that ATM it just improve this for Core protocol, but with some help, will get the same for AMQP as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577050668
@clebertsuconic The downside of this optimization is that it still need to read the whole part of buffer with the properties in the best case: the ideal thing would be to always place `ScheduledDeliveryTime` in a fixed position (as first property?)
I see that the last change on journal hasn't lowered the CI test time, but probably increased it: I have yet to verify it by trying the previous commit.
And, in addition, I see there are some test failures, intermittent, but these next 3 days I will travel and canno verify if the test failures are real and due to some of my latest changes (think not TBH)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369652515
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
wouldn't be less costly to just lookup for the sequence of bytes?
You're still parsing the key at its exact position.
if just looking for the sequence , say _HDR_SQUEDULED_TIME anywhere in the buffer would be enough.
now if the search is more costly than the partial parsing, then it's fine.
At least you're not reading the string to verify the equal object.
if this was C, you would control to reuse the same integer every time, but in Java every time you do buffer.getInt(index) you will create a few bytes in memory.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-580207415
@michaelandrepearce @clebertsuconic I've added some tests and this is ready to get a full review.
After addressing all the review comments I'll squash the changes into one. The CI is already in a good shape on this :+1:
There is just one thing that worries me about these optimizations and is related to theirs fragility: how to ensure that future changes in the journal loading process won't trigger by accident the encoding of the message? That was a major cause of performance issue
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369657571
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
>> "the risk is to find a value that matches the property we are searching for"
So what? if someone uses the key as the value, all it will incur is for us to do the older semantic, which is to parse the whole buffer. and we would incur in the old semantic.
Add a test with the value as the key, just to make sure.. although I doubt someone will use the property _HDR_SCHEDULED_TIME anywhere in the values :) and if they did it would still be fine.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2950: ARTEMIS-2604
Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 52 seconds
Is about twice as faster assuming a very fast disk :)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
It seems about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11 i have to raise it to 6 GB to make the loading quick as this pr with GB.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on issue #2950:
ARTEMIS-2604 Optimizations on journal loading
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on issue #2950: ARTEMIS-2604 Optimizations on journal loading
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-582487500
I'm merging this
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request
#2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369659026
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
But is not faster according my tests...in one case we perform more comparisons...
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 commented on issue #2950: ARTEMIS-2604
Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 commented on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-577046495
@clebertsuconic Sadly seems that while exchanging messages, we trigger the full decode anyway, due to other properties accessed eg `CoreMessage:.getRoutingType`
But, good news about the change already done:
that's from *ARTEMIS 2.11*
And the same heap configuration ie `-XX:+UseG1GC -Xms4G -Xmx4G -XX:+AlwaysPreTouch` for *this PR*:
The difference in pause time is just too much to compare them :)
There are other metrics, but the point is that we won't produce tons of garbage to decode messages if the `ScheduledDeliveryTime` property isn't set and should cover the most of the cases.
The previous results has been performed by loading a journal obtained by running 30 pairs of Core JMS producers/consumers sending persistent byte messages of 10 bytes size until the journal start to page, with a slow consumer (that perform Thread::sleep(200) while consuming messages): this has filled the message journal with a mix of unacked/acked small messages that would stress the heap/GC part of the loading activity instead of the disk.
Indeed the whole `data` folder is just `2.5 GB`.
I would try with different sizes too, to put under stress the new search property method, that is being benchmarked separately to make it as fast as possible.
I strongly encourage any user that make heavy use of the journal and has stress tests while loading journal to try this, being aware that ATM it just improve this for Core protocol, but with some help, will get the same for AMQP as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull
request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#discussion_r369655637
##########
File path: artemis-commons/src/main/java/org/apache/activemq/artemis/utils/collections/TypedProperties.java
##########
@@ -372,6 +372,79 @@ private synchronized void forEachInternal(BiConsumer<SimpleString, PropertyValue
}
}
+ /**
+ * Performs a search among the valid key properties contained in {@code buffer}, starting from {@code from}
+ * assuming it to be a valid encoded {@link TypedProperties} content.
+ *
+ * @throws IllegalStateException if any not-valid property is found while searching the {@code key} property
+ */
+ public static boolean searchProperty(SimpleString key, ByteBuf buffer, int startIndex) {
Review comment:
if you could name this as hasProperty(SimpleString key).
since you use the parsed property if the buffer is not parsed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
It seems about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB if journal and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11 i have to raise it to 6 GB to make the loading quick as this pr with 3 GB. And I still need to take care of Long boxing, probably in a follow up pr ;)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [activemq-artemis] franz1981 edited a comment on issue #2950:
ARTEMIS-2604 Save Message decoding when unnecessary
Posted by GitBox <gi...@apache.org>.
franz1981 edited a comment on issue #2950: ARTEMIS-2604 Save Message decoding when unnecessary
URL: https://github.com/apache/activemq-artemis/pull/2950#issuecomment-581056951
@michaelandrepearce @wy96f I finally got some numbers about the loading of journal:
this commit:
start: 19:28:08,193
end: 19:28:28,391
~20 seconds
2.11:
start: 12:12:22,651
end: 12:13:15,625
~ 53 seconds
It seems about twice as faster assuming a very fast disk :)
But, although it seems an improvement from the point of view of speed of the algorithm of loading, in reality, it uses much less memory then before, hence hitting much less full GCs.
In my tests, with. 2.5 GB and a 4 GB broker, I can lower the heap size until 3 GB getting decent performance while loading, while with 2.11 i have to raise it to 6 GB to make the loading quick as this pr with 3 GB. And I still need to take care of Long boxing, probably in a follow up pr ;)
----------------------------------------------------------------
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
With regards,
Apache Git Services