Outbound NormalResponse optimizations #10318
Conversation
pveentjer
changed the title
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
from
f446c89
to
a8b0887
Compare
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
from
e920ce6
to
7533843
Compare
|
run-lab-run |
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
2 times, most recently
from
96be2f1
to
25fbe92
Compare
| * But if you want to serialize an object to a byte-array and don't care for the Data partition-hash, the hash can be | ||
| * disabled. | ||
| */ | ||
| byte[] toBytes(Object obj, int leftPadding, boolean insertPartitionHash); |
There was a problem hiding this comment.
+1 for the method and javadoc cleanup
| } | ||
|
|
||
| Packet toNormalResponsePacket(long callId, int backupAcks, boolean urgent, Object value) { | ||
| byte[] bytes; |
There was a problem hiding this comment.
So now the NormalResponse#writeData should not be called? Can we add some note or comment that we have this custom serialization? Can it throw an exception?
I like the optimisation, I'm just worrying that someone forgets that we duplicate the logic from the DataSerializableSerializer and EnterpriseDataSerializableSerializer here.
When someone makes some changes in those and forgets to change this serialization here, probably we will get failing tests (which is good).
Maybe just add some comments in the serializers there? Or make the NormalResponse not IDS and have it's serialization and deserialization fixed here?
Another idea - we move this into a new method on the internal serialization service?
There was a problem hiding this comment.
They can still be called; I'm using them even to verify if the 'optimized' serialization does the same thing as serialization NormalResponse.
Eventually I would like to drop all the responses since they are not truly needed; but I tried this a few time and it leads to a lot of code change.
I like the optimisation, I'm just worrying that someone forgets that we duplicate the logic from the DataSerializableSerializer and EnterpriseDataSerializableSerializer here.
They can't be changed.. at least not in the 3.x timeline due to minor version compatibility.
Another idea - we move this into a new method on the internal serialization service?
I don't think we should get a dependency of something as specific as response into something generic as serialization. It would cause a cycle (tight coupling).
There was a problem hiding this comment.
Ok, good that we have a test meaning that it will fail if someone tries to change the serialization format and forgets to change it in all places. Looking forward to moving the responses away from IDS and more internally.
There was a problem hiding this comment.
Can we still remove them at all? They are part of 3.8 and with rolling upgrade we still have to support them (at least one minor version). Otherwise this should go on the 4.0 list.
There was a problem hiding this comment.
The actual data is going to remain the same. Only we don't need to have 'physical' response classes.
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
2 times, most recently
from
ae2138e
to
15fcecf
Compare
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
from
15fcecf
to
e458bd8
Compare
|
Test PASSed. |
| //call id | ||
| writeLong(bytes, OFFSET_CALL_ID, callId, useBigEndian); | ||
| // urgent | ||
| bytes[OFFSET_URGENT] = (byte) (urgent ? 1 : 0); |
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
2 times, most recently
from
1309be9
to
0c1b91d
Compare
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
from
0c1b91d
to
98c2808
Compare
|
run-lab-run |
Skips the NormalResponse creation in case of responses for responses that don't require a ack from a backup. Most importantly: it also skips an intermediate copy of the heap/native-data into a temporary serialization buffer and then into the packet. With this change the data is immediately copied into the packet-bytes. This pr is nativememorydata friendly; it doesn't cause unwanted call to toByteArray.
pveentjer
force-pushed
the
v3.9/performance/outbound-normalresponse-optimizations
branch
from
98c2808
to
c687fac
Compare
|
run-lab-run |
|
Test PASSed. |
|
verify |
|
Test FAILed. |
|
run-lab-run |
|
Test PASSed. |
pveentjer
deleted the
v3.9/performance/outbound-normalresponse-optimizations
branch
This PR contains the following optimizations
This PR is native-memory friendly; it doesn't force the NativeMemoryData to create a copy onheap.
This is a follow up PR for
#10317
There is no change in the data that goes over the wire. So there will not be any minor version incompatibilities.
fix #10285