[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
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 issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
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...
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. ---
[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...
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...
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...
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...
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...
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...
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...
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...
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: ---
