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/04/19 16:11:48 UTC
[GitHub] activemq-artemis pull request #2031: ARTEMIS-1816 OpenWire should avoid Byte...
GitHub user franz1981 opened a pull request:
https://github.com/apache/activemq-artemis/pull/2031
ARTEMIS-1816 OpenWire should avoid ByteArrayOutputStream lazy allocation
OpenWireMessageConverter::toAMQMessage on bytes messages is lazy
allocating a write buffer with a default size of 1024 even when
it won't be used to write anything.
It avoid an useless allocation by reducing it to new byte[0].
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1816
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/2031.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 #2031
----
commit a391704c54d1d73e0dcc57e5c71a384e6299e3cd
Author: Francesco Nigro <ni...@...>
Date: 2018-04-19T15:46:26Z
ARTEMIS-1816 OpenWire should avoid ByteArrayOutputStream lazy allocation
OpenWireMessageConverter::toAMQMessage on bytes messages is lazy
allocating a write buffer with a default size of 1024 even when
it won't be used to write anything.
It avoid an useless allocation by reducing it to new byte[0].
----
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
Ok, I'll be "the decider". Will check with franz to see how much it's worth to him
---
[GitHub] activemq-artemis pull request #2031: ARTEMIS-1816 OpenWire should avoid Byte...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/2031
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
@clebertsuconic I need to run all the CI tests on it to be safe :+1:
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
@tabish121 I'm sorry to have "hacked" like this the Open Wire class. The best solution would be to avoid at all any allocation of `ActiveMQBytesMessage`, any idea?
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
That would be the more ideal thing to do yes.
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
for optimizing OpenWire.. I would prefer implementing OpenWireMessage properly. There's a PR open for that already.
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
@tabish121 no regressions on the testsuites.. I will let you decide about merging this or not.
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
@franz1981 So I think that you could avoid subclassing the ActiveMQBytesMessage if you built the properties yourself instead of calling setObjectProperty in setAMQMsgObjectProperties in the converter as that is where the trigger point is for allocating the bytes stream (see initializeWriting and its uses in ActiveMQBytesMessage). In a manner similar to how the converter puts the body into the message using setContent you could create and marshal the properties and store them using setMarshalledProperties in the OpenWire base Message class. The properties are marshalled using the utility class MarshallingSupport#marshalPrimitiveMap so it is possible to roll your own and then bypass that bit of the ActiveMQMessage's normal handling.
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by franz1981 <gi...@git.apache.org>.
Github user franz1981 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
I agree but given the short window until a new release i have preferred to push this (that's quite stable and safe) and taking my time to improve the other optimal way. That pr currently broke (last time I have checked) too many tests and I think it worth to make it stable without rushing.
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
@franz1981 sorry.. but I -1 on this.. you ok with closing this?
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
The change as it is seems acceptable as it does reduce unnecessary allocations when ByteMessages types cross to OpenWire. I've reviewed the changes as it relates to the OpenWire objects and the way the converter uses it and I don't see anything adverse happening from this. As always a run through the tests would be good for some validation.
---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
Posted by clebertsuconic <gi...@git.apache.org>.
Github user clebertsuconic commented on the issue:
https://github.com/apache/activemq-artemis/pull/2031
@tabish121 @franz1981 fair enough.. lets run the whole testsuite then.. then we can merge this.
---