You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by franz1981 <gi...@git.apache.org> on 2018/08/28 14:05:25 UTC
[GitHub] activemq-artemis pull request #2274: ARTEMIS-2059 NettyWritable should use U...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/2274
ARTEMIS-2059 NettyWritable should use UTF-8 exact length to encode strings
NettyWritable.put(String) tries to enlarge the buffer used to write a UTF-8 string until ByteBufUtil.utf8MaxBytes.
That means that it will fail or will enlarge any ByteBuf that is perfectly sized to contain the encoded string.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-2059
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2274.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 #2274
----
commit c5dda70250cc7cafb5779e99ad9f70619afc964d
Author: Francesco Nigro <ni...@...>
Date: 2018-08-28T13:57:19Z
ARTEMIS-2059 NettyWritable should use UTF-8 exact length to encode strings
----
---
[GitHub] activemq-artemis pull request #2274: ARTEMIS-2059 NettyWritable should use U...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2274#discussion_r214067838
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/util/NettyWritable.java ---
@@ -106,7 +105,8 @@ public void put(ByteBuf payload) {
@Override
public void put(String value) {
- nettyBuffer.writeCharSequence(value, StandardCharsets.UTF_8);
--- End diff --
@clebertsuconic AFAIK this is happening on Core, but with AMQP the Strings are UTF-8 encoded using `WritableBuffer::put(String)`.
I'm not 100% sure of it: @gemmellr do you know for what it is used `WritableBuffer::put(String)`?
---
[GitHub] activemq-artemis issue #2274: ARTEMIS-2059 NettyWritable should use UTF-8 ex...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2274
@gemmellr It is a good precisation indeed: it is more an improvement, but if have just allocated fixed size ByteBuf it will fail throwing an exception even if the space is enough, that's why I have marked it as a minor bug, but I can change it on the JIRA...
---
[GitHub] activemq-artemis issue #2274: ARTEMIS-2059 NettyWritable should use UTF-8 ex...
Posted by jbertram <gi...@git.apache.org>.
Github user jbertram commented on the issue:
https://github.com/apache/activemq-artemis/pull/2274
@franz1981, @gemmellr, is this ready to be closed or is further discussion needed?
---
[GitHub] activemq-artemis pull request #2274: ARTEMIS-2059 NettyWritable should use U...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2274#discussion_r213379239
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/util/NettyWritable.java ---
@@ -106,7 +105,8 @@ public void put(ByteBuf payload) {
@Override
public void put(String value) {
- nettyBuffer.writeCharSequence(value, StandardCharsets.UTF_8);
--- End diff --
utf8Bytes will make calculations to send Strings.
This is at least changing the wire.
Basically we value performance with less UTf8 calculations.. sending pairs for strings, right? or am I missing something?
I'm worried about this also breaking string compatibiility.
---
[GitHub] activemq-artemis issue #2274: ARTEMIS-2059 NettyWritable should use UTF-8 ex...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2274
@tabish121 @gemmellr I will be probably able to take a further look on it after wednesday...
Anyway I think that this PR as it is will just fix the bug at a performance cost: ByteBufUtil.utf8Bytes has a O(n) cost while ByteBufUtil.utf8MaxBytes is O(1).
A trivial fix with no performance hit that assume that the ByteBuffer has been sized with enough space to hold the encoded string would be to use ```ByteBufUtil.reserveAndWriteUtf8(nettyBuffer, value, nettyBuffer.writableBytes());``` instead, but I'm not sure if NettyWritable::put(String) can safely assume it.
---
[GitHub] activemq-artemis issue #2274: ARTEMIS-2059 NettyWritable should use UTF-8 ex...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/2274
Not sure I'd call it a bug really, more that this is a memory optimisation, one that shouldnt be getting used much since it only comes into play if you are encoding things (whereas it should mostly be passing stuff through), in which case its likely to be expanding the buffer anyway given typical usage.
---
[GitHub] activemq-artemis pull request #2274: ARTEMIS-2059 NettyWritable should use U...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2274#discussion_r214073404
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/util/NettyWritable.java ---
@@ -106,7 +105,8 @@ public void put(ByteBuf payload) {
@Override
public void put(String value) {
- nettyBuffer.writeCharSequence(value, StandardCharsets.UTF_8);
--- End diff --
Any string encoded by proton-j will use the method. The broker tries not to encode things for AMQP->AMQP cases however so a lot of the time it wont be used at all. The rest of the time, it is specifically using an expanding buffer as it doesn't know the size of whats being encoded.
---
[GitHub] activemq-artemis issue #2274: ARTEMIS-2059 NettyWritable should use UTF-8 ex...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2274
@gemmellr You are right: indeed I'm thinking if it makes sense to close this considering that it will cost some perf..
I'm not aware of the lifecycle of the netty buffer used: if it has been sized with a precise size and won't be reused, it will cost the enlarging of it ( a copy that will cost the copy of the size of the data already encoded + gerbage a new allocation) and it is not desiderable I think.
what do you suggest?
---
[GitHub] activemq-artemis issue #2274: ARTEMIS-2059 NettyWritable should use UTF-8 ex...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the issue:
https://github.com/apache/activemq-artemis/pull/2274
Fair enough, I can see that could be argued as a bug, however are there any uses in the broker which will do that? I don't know of any off hand.
---