You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "rpuch (via GitHub)" <gi...@apache.org> on 2024/11/07 13:36:24 UTC
[PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
rpuch opened a new pull request, #4689:
URL: https://github.com/apache/ignite-3/pull/4689
https://issues.apache.org/jira/browse/IGNITE-23631
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689#discussion_r1833854205
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
+ for (ClassDescriptorMessage classDescriptorMessage : outObject.descriptors()) {
Review Comment:
From my experience, I can say that I came across a situation in projects when we started to see a lot of iterator allocations in the product profiler in places where this should have been optimized as well. I assume that this could happen because JIT threw out optimizations for various reasons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689#discussion_r1833826339
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
+ for (ClassDescriptorMessage classDescriptorMessage : outObject.descriptors()) {
Review Comment:
I'll assume that it is being created but it's just not visible. Your ticket is about optimization, let's do it to the maximum, the counting loop in this case will make 1 line of code more, which will not lose the readability of the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689#discussion_r1833810359
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
+ for (ClassDescriptorMessage classDescriptorMessage : outObject.descriptors()) {
Review Comment:
It is possible, but foreach loop doesn't perform worse than the classic `for` loop on indexes. I checked allocations in the profiler, the iterator is not even allocated (probably, escape analysis does its trick). On the other hand, foreach loop is more readable and less prone to coding errors (less moving parts are in the code where we can screw up)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689#discussion_r1833848197
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
+ for (ClassDescriptorMessage classDescriptorMessage : outObject.descriptors()) {
Review Comment:
It is certainly not created on the heap, so what happens is allocation of a variable on the stack. The same thing that is done in an indexed for loop, but more readable and more difficult to break.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689#discussion_r1833916762
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
+ for (ClassDescriptorMessage classDescriptorMessage : outObject.descriptors()) {
Review Comment:
Changed to indexed for loop
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch merged PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689#discussion_r1833777378
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
Review Comment:
Doesn't concern your changes, this is not the first time we've used a lazy list, it's probably time to create a class for it.
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
+ for (ClassDescriptorMessage classDescriptorMessage : outObject.descriptors()) {
Review Comment:
Is it possible to change to a counting cycle?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -119,20 +125,20 @@ private NetworkMessageChunkedInput(
/** {@inheritDoc} */
Review Comment:
Maybe we should get rid of useless comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-23631 Optimize NetworkMessageChunkedInput constructor [ignite-3]
Posted by "rpuch (via GitHub)" <gi...@apache.org>.
rpuch commented on code in PR #4689:
URL: https://github.com/apache/ignite-3/pull/4689#discussion_r1834097038
##########
modules/network/src/main/java/org/apache/ignite/internal/network/netty/OutboundEncoder.java:
##########
@@ -98,11 +98,17 @@ private NetworkMessageChunkedInput(
this.serializationService = serializationService;
this.msg = outObject.networkMessage();
- List<ClassDescriptorMessage> outDescriptors = outObject.descriptors().stream()
- .filter(classDescriptorMessage -> !serializationService.isDescriptorSent(classDescriptorMessage.descriptorId()))
- .collect(Collectors.toList());
+ List<ClassDescriptorMessage> outDescriptors = null;
Review Comment:
The only option that comes to my mind is using some kind of a supplier to introduce lazyness, but using lambdas in such a way seems to be scary for hot code. What do you have in mind?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org