You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by franz1981 <gi...@git.apache.org> on 2018/11/17 21:19:29 UTC
[GitHub] qpid-proton-j pull request #20: PROTON-1965 Optimize CompositeReadableBuffer...
GitHub user franz1981 opened a pull request:
https://github.com/apache/qpid-proton-j/pull/20
PROTON-1965 Optimize CompositeReadableBuffer::equals with single chunk
Using the single chunk directly while performing the byte comparison increase the performance.
Master:
```
Benchmark (chunks) (direct) (size) Mode Cnt Score Error Units
CompositeReadableBufferBenchmark.equalsToByteBufferReader 1 false 8 avgt 5 18.127 ± 1.568 ns/op
CompositeReadableBufferBenchmark.equalsToByteBufferReader 1 false 64 avgt 5 40.725 ± 2.329 ns/op
CompositeReadableBufferBenchmark.equalsToByteBufferReader 1 false 1024 avgt 5 460.801 ± 13.786 ns/op
```
This PR:
```
Benchmark (chunks) (direct) (size) Mode Cnt Score Error Units
CompositeReadableBufferBenchmark.equalsToByteBufferReader 1 false 8 avgt 5 27.799 ± 0.462 ns/op
CompositeReadableBufferBenchmark.equalsToByteBufferReader 1 false 64 avgt 5 108.563 ± 1.236 ns/op
CompositeReadableBufferBenchmark.equalsToByteBufferReader 1 false 1024 avgt 5 5251.102 ± 261.561 ns/op
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/qpid-proton-j PROTON-1965
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/qpid-proton-j/pull/20.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #20
----
commit d4932b415fa73ff3378ca19af46c51fcf15d84de
Author: Francesco Nigro <ni...@...>
Date: 2018-11-17T18:59:12Z
PROTON-1965 Optimize CompositeReadableBuffer::equals with single chunk
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
Checked in with infra who looked into it and found the issue, mirror is now back up to date.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
@franz1981 Looks good so far, would like to see the formatting changed to match the remainder of the code. The reset is a good catch, that is a bug in the current code.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
For later reference, a related further change was made via b3a66b741c34b40b059fc023b3c7650720dfc001 to apply the optimisation in more cases.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #20: PROTON-1965 Optimize CompositeReadableBuffer...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/20#discussion_r234421816
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/codec/CompositeReadableBuffer.java ---
@@ -825,33 +825,67 @@ public int hashCode() {
}
@Override
- public boolean equals(Object other) {
- if (this == other) {
+ public boolean equals(Object other)
+ {
+ if (this == other)
+ {
return true;
}
- if (!(other instanceof ReadableBuffer)) {
+ if (!(other instanceof ReadableBuffer))
+ {
return false;
}
- ReadableBuffer buffer = (ReadableBuffer)other;
- if (this.remaining() != buffer.remaining()) {
+ ReadableBuffer buffer = (ReadableBuffer) other;
+ final int remaining = remaining();
+ if (remaining != buffer.remaining())
+ {
return false;
}
+ if (hasArray())
+ {
+ return equals(currentArray, position, remaining, buffer);
+ }
+ else
+ {
+ return equals(this, buffer);
+ }
+ }
- final int currentPos = position();
-
- for (int i = buffer.position(); hasRemaining(); i++) {
- if (!equals(this.get(), buffer.get(i))) {
+ private static boolean equals(byte[] buffer, int start, int length, ReadableBuffer other)
+ {
+ final int position = other.position();
+ for (int i = 0; i < length; i++)
+ {
+ if (buffer[start + i] != other.get(position + i))
+ {
return false;
}
}
-
- position(currentPos);
-
return true;
}
+ private static boolean equals(ReadableBuffer buffer, ReadableBuffer other)
+ {
+ final int currentPos = buffer.position();
+ try
+ {
+ for (int i = other.position(); buffer.hasRemaining(); i++)
+ {
+ if (!equals(buffer.get(), other.get(i)))
+ {
+ return false;
+ }
+ }
+ return true;
+ }
+ finally
+ {
+ buffer.position(currentPos);
--- End diff --
@tabish121 I have added a reset of the position that wasn't present in the original code, I hope that's correct
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
It seems the github mirror is not up to date, and I've since prodded a re-sync and it still isn't up to date. I'll possibly need to ask infra about it after lunch.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
The reset is a good catch. Its a noteworthy bug in its own right and so should be fixed separately, and also tested. I have done that now via PROTON-1966. Sorry for the hassle @franz1981 but can you please rebase your PR again.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #20: PROTON-1965 Optimize CompositeReadableBuffer...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/20#discussion_r234714952
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/codec/CompositeReadableBuffer.java ---
@@ -834,22 +834,39 @@ public boolean equals(Object other) {
return false;
}
- ReadableBuffer buffer = (ReadableBuffer)other;
- if (this.remaining() != buffer.remaining()) {
+ ReadableBuffer buffer = (ReadableBuffer) other;
+ final int remaining = remaining();
+ if (remaining != buffer.remaining()) {
return false;
}
+ if (hasArray()) {
+ return equals(currentArray, position, remaining, buffer);
--- End diff --
This bit is a little broken as it uses position without account for any array offset in the current buffer. Its position in the buffer may not be the same as its position within the array, e.g. if it were previously sliced from a larger buffer. Calling equals with slices from two equal buffers with different current positions will fail.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #20: PROTON-1965 Optimize CompositeReadableBuffer...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/20#discussion_r234719950
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/codec/CompositeReadableBuffer.java ---
@@ -834,22 +834,39 @@ public boolean equals(Object other) {
return false;
}
- ReadableBuffer buffer = (ReadableBuffer)other;
- if (this.remaining() != buffer.remaining()) {
+ ReadableBuffer buffer = (ReadableBuffer) other;
+ final int remaining = remaining();
+ if (remaining != buffer.remaining()) {
return false;
}
+ if (hasArray()) {
+ return equals(currentArray, position, remaining, buffer);
--- End diff --
So ti should take into account the arrayOffset and do not rely on position?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
I've pushed an updated version of the change with a fix for the offset issue I noticed yesterday, which Tim added a test to cover.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
Can you unwind the 'revert' of my currentPos -> origPos variable name change from earlier? :)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #20: PROTON-1965 Optimize CompositeReadableBuffer...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/qpid-proton-j/pull/20
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j issue #20: PROTON-1965 Optimize CompositeReadableBuffer::equal...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/qpid-proton-j/pull/20
@gemmellr np Robbie! I believe that's better to have it handled separately :+1:
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] qpid-proton-j pull request #20: PROTON-1965 Optimize CompositeReadableBuffer...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on a diff in the pull request:
https://github.com/apache/qpid-proton-j/pull/20#discussion_r234811453
--- Diff: proton-j/src/main/java/org/apache/qpid/proton/codec/CompositeReadableBuffer.java ---
@@ -834,22 +834,39 @@ public boolean equals(Object other) {
return false;
}
- ReadableBuffer buffer = (ReadableBuffer)other;
- if (this.remaining() != buffer.remaining()) {
+ ReadableBuffer buffer = (ReadableBuffer) other;
+ final int remaining = remaining();
+ if (remaining != buffer.remaining()) {
return false;
}
+ if (hasArray()) {
+ return equals(currentArray, position, remaining, buffer);
--- End diff --
For the single array case, when you have a slice that you've created from a CRB that is not starting from zero then the current offset will be non-zero but position would be so the equals check should be starting at current offset and moving forward however many elements the limit allows for.
The position and the offset work together to control where zero is in the buffer, you can do a get for some index in the buffer and the position controls how far back you can rewind the offset until you've hit "zero" which might not be zero in the array.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org