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/07/17 11:01:10 UTC
[GitHub] [activemq-artemis] franz1981 opened a new pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
franz1981 opened a new pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223
See https://issues.apache.org/jira/browse/ARTEMIS-2849
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r457515880
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
@franz1981 the failure is real... this PR is broken.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456498525
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
Every borrowed pooled buffer always start at position 0, so the observer, while flushing always overwrite exactly the bytes it has requested despite being zeroed or not
> Also: did you run the whole test suite on such change?
Yet to do it, will do it now :)
NIO and MAPPED already use this optimization from long time (the jira link explain it): ASYNCIO was the only one missing yet
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456498525
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
> . I'm afraid this could not cleanup the rest of the 4K
In the original code of ASYNCIO it wasn't clearing the rest of 4K bytes, see https://github.com/apache/activemq-artemis-native/blob/master/src/main/java/org/apache/activemq/artemis/nativo/jlibaio/LibaioContext.java#L134
The original.code was clearing just the required new capacity (that's 300 in your example).
> Also: did you run the whole test suite on such change?
Yet to do it, will do it now :)
NIO and MAPPED already use this optimization from long time (the jira link explain it): ASYNCIO was the only one missing yet
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#issuecomment-663096890
squash the commits and I will merge 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
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456498525
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
Every borrowed pooled buffer always start at position 0, so the observer, while flushing always overwrite exactly the bytes it has requested despite being zeroed or not: the case you've mentioned can happen only if ASYNCIO would use some thread local pool of ByteBuffer like MAPPED or NIO are doing, but ASYNCIO is using a `ConcurrentLinkedQueue`-based pool instead
> Also: did you run the whole test suite on such change?
Yet to do it, will do it now :)
NIO and MAPPED already use this optimization from long time (the jira link explain it): ASYNCIO was the only one missing yet
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
franz1981 commented on pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#issuecomment-662347163
@clebertsuconic The fix seems to work fine, but I'm not very proud of the solution: if we want to write 128 bytes and the block size is 4096, `bufferControl` allocates (or can reuse) a 4096 sized ByteBuffer without zeroing it (that's nice), but the journal record still need to zero `4096 - 128` bytes to save having garbage data on the journal. If we write enough data this optimization can save lot of zeroing but cannot eliminate it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456494094
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
Also: did you run the whole test suite on such change?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#issuecomment-662459390
that's a result of reusing the buffers from previous writes. Which is what I mentioned to you.
on Mapped and NIO there's no alignment, meaning you don't need to set zeroes to a previously reused buffer.
With libaio we have our own pool... I think this is expected.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456498525
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
> . I'm afraid this could not cleanup the rest of the 4K
In the original code of ASYNCIO it was already non clearing the rest of 4K bytes, see https://github.com/apache/activemq-artemis-native/blob/master/src/main/java/org/apache/activemq/artemis/nativo/jlibaio/LibaioContext.java#L134
The original.code was clearing just the required new capacity (that's 300 in your example).
> Also: did you run the whole test suite on such change?
Yet to do it, will do it now :)
NIO and MAPPED already use this optimization from long time (the jira link explain it): ASYNCIO was the only one missing yet
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456492114
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
In case we pool a buffer, and say you wrote 300 bytes, on a place that previously had 4K filled
you will fill it with 300 bytes... the buffer will be flipped, but when the libaio write takes place.. I'm afraid this could not cleanup the rest of the 4K.
I did not run any tests on this.. just from what I remember on the pool mechanism.
just make sure the previously pooled buffer wouldn't make to the journal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456498525
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
Every borrowed pooled buffer always start at position 0, so the observer, while flushing always overwrite exactly the bytes it has requested despite being zeroed or not: the case you've mentioned can happen only if ASYNCIO would use some thread local pool of ByteBuffers (as MAPPED or NIO does) but ASYNCIO is using a `ConcurrentLinkedQueue`-based pool instead
> Also: did you run the whole test suite on such change?
Yet to do it, will do it now :)
NIO and MAPPED already use this optimization from long time (the jira link explain it): ASYNCIO was the only one missing yet
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r457453521
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
I actually had access to the same build as you, and as I can see the build did not complete.. lots of failures due to open files. I will restart that build
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3223: ARTEMIS-2849 Eliminate zeroing of buffers while writing the ASYNCIO journal
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3223:
URL: https://github.com/apache/activemq-artemis/pull/3223#discussion_r456580099
##########
File path: artemis-journal/src/main/java/org/apache/activemq/artemis/core/io/AbstractSequentialFile.java
##########
@@ -271,7 +271,7 @@ protected ByteBuffer newBuffer(int size, int limit) {
public void flushBuffer(final ByteBuf byteBuf, final boolean requestedSync, final List<IOCallback> callbacks) {
final int bytes = byteBuf.readableBytes();
if (bytes > 0) {
- final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes);
+ final ByteBuffer buffer = newBuffer(byteBuf.capacity(), bytes, false);
Review comment:
About the tests: I see that only this one has failed
```
Error Message
expected:<10> but was:<11>
Stacktrace
java.lang.AssertionError: expected:<10> but was:<11>
at org.apache.activemq.artemis.tests.unit.core.postoffice.impl.DuplicateDetectionUnitTest.testReloadDuplication(DuplicateDetectionUnitTest.java:127)
```
And probably it isn't related
----------------------------------------------------------------
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