BitoAgent commented on code in PR #13786:
URL: https://github.com/apache/dubbo/pull/13786#discussion_r1573858746
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcCompositeCodec.java:
##########
@@ -16,123 +16,103 @@
*/
package org.apache.dubbo.rpc.protocol.tri.h12.grpc;
+import org.apache.dubbo.common.URL;
+import org.apache.dubbo.common.config.ConfigurationUtils;
+import org.apache.dubbo.common.io.StreamUtils;
+import org.apache.dubbo.common.utils.ArrayUtils;
import org.apache.dubbo.remoting.http12.exception.DecodeException;
import org.apache.dubbo.remoting.http12.exception.EncodeException;
import org.apache.dubbo.remoting.http12.message.HttpMessageCodec;
import org.apache.dubbo.remoting.http12.message.MediaType;
+import org.apache.dubbo.rpc.model.FrameworkModel;
+import org.apache.dubbo.rpc.model.MethodDescriptor;
+import org.apache.dubbo.rpc.model.PackableMethod;
+import org.apache.dubbo.rpc.model.PackableMethodFactory;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.charset.Charset;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
-import com.google.protobuf.Message;
-
-import static
org.apache.dubbo.common.constants.CommonConstants.PROTOBUF_MESSAGE_CLASS_NAME;
+import static org.apache.dubbo.common.constants.CommonConstants.DEFAULT_KEY;
+import static
org.apache.dubbo.common.constants.CommonConstants.DUBBO_PACKABLE_METHOD_FACTORY;
public class GrpcCompositeCodec implements HttpMessageCodec {
- private final ProtobufHttpMessageCodec protobufHttpMessageCodec;
+ private static final String PACKABLE_METHOD_CACHE =
"PACKABLE_METHOD_CACHE";
- private final WrapperHttpMessageCodec wrapperHttpMessageCodec;
+ private final URL url;
- public GrpcCompositeCodec(
- ProtobufHttpMessageCodec protobufHttpMessageCodec,
WrapperHttpMessageCodec wrapperHttpMessageCodec) {
- this.protobufHttpMessageCodec = protobufHttpMessageCodec;
- this.wrapperHttpMessageCodec = wrapperHttpMessageCodec;
- }
+ private final FrameworkModel frameworkModel;
+
+ private final String mediaType;
- public void setEncodeTypes(Class<?>[] encodeTypes) {
- this.wrapperHttpMessageCodec.setEncodeTypes(encodeTypes);
+ private PackableMethod packableMethod;
+
+ public GrpcCompositeCodec(URL url, FrameworkModel frameworkModel, String
mediaType) {
+ this.url = url;
+ this.frameworkModel = frameworkModel;
+ this.mediaType = mediaType;
}
- public void setDecodeTypes(Class<?>[] decodeTypes) {
- this.wrapperHttpMessageCodec.setDecodeTypes(decodeTypes);
+ public void loadPackableMethod(MethodDescriptor methodDescriptor) {
+ if (methodDescriptor instanceof PackableMethod) {
+ packableMethod = (PackableMethod) methodDescriptor;
+ return;
+ }
+ Map<MethodDescriptor, PackableMethod> cacheMap =
(Map<MethodDescriptor, PackableMethod>) url.getServiceModel()
+ .getServiceMetadata()
+ .getAttributeMap()
+ .computeIfAbsent(PACKABLE_METHOD_CACHE, k -> new
ConcurrentHashMap<>());
+ packableMethod = cacheMap.computeIfAbsent(methodDescriptor, md ->
frameworkModel
+ .getExtensionLoader(PackableMethodFactory.class)
+
.getExtension(ConfigurationUtils.getGlobalConfiguration(url.getApplicationModel())
+ .getString(DUBBO_PACKABLE_METHOD_FACTORY, DEFAULT_KEY))
+ .create(methodDescriptor, url, mediaType));
Review Comment:
**Optimization Issue**: The implementation of loading and caching
PackableMethod instances could be optimized for concurrent access patterns. The
current use of ConcurrentHashMap.computeIfAbsent is efficient but still
involves multiple steps and lambda expressions which could be streamlined
further for high-throughput scenarios. <br> **Fix**: Consider using a more
direct caching strategy that minimizes lambda creation for each computeIfAbsent
call. One approach could be to pre-load PackableMethods for known
MethodDescriptors during the initialization phase of GrpcCompositeCodec, thus
avoiding the need for computeIfAbsent during the critical path of method
invocation. <br> **Code Suggestion**:
```
+ Map<MethodDescriptor, PackableMethod> cacheMap =
preloadedPackableMethods;
+ packableMethod = cacheMap.get(methodDescriptor);
+ if (packableMethod == null) {
+ packableMethod = loadPackableMethod(methodDescriptor);
+ cacheMap.put(methodDescriptor, packableMethod);
+ }
```
##########
dubbo-rpc/dubbo-rpc-triple/src/main/java/org/apache/dubbo/rpc/protocol/tri/h12/grpc/GrpcHttp2ServerTransportListener.java:
##########
@@ -145,39 +146,31 @@ public void onMessage(InputStream inputStream) {
private class DetermineMethodDescriptorListener implements
StreamingDecoder.FragmentListener {
- @Override
- public void onFragmentMessage(InputStream rawMessage) {}
-
@Override
public void onClose() {
getStreamingDecoder().close();
}
@Override
- public void onFragmentMessage(InputStream dataHeader, InputStream
rawMessage) {
+ public void onFragmentMessage(InputStream rawMessage) {
try {
- ByteArrayOutputStream merged =
- new ByteArrayOutputStream(dataHeader.available() +
rawMessage.available());
- StreamUtils.copy(dataHeader, merged);
- byte[] data = StreamUtils.readBytes(rawMessage);
-
RpcInvocationBuildContext context = getContext();
if (null == context.getMethodDescriptor()) {
-
context.setMethodDescriptor(DescriptorUtils.findTripleMethodDescriptor(
- context.getServiceDescriptor(),
context.getMethodName(), data));
+ byte[] data = StreamUtils.readBytes(rawMessage);
+ MethodDescriptor methodDescriptor =
DescriptorUtils.findTripleMethodDescriptor(
+ context.getServiceDescriptor(),
context.getMethodName(), data);
+ context.setMethodDescriptor(methodDescriptor);
setHttpMessageListener(GrpcHttp2ServerTransportListener.super.buildHttpMessageListener());
// replace decoder
GrpcCompositeCodec grpcCompositeCodec =
(GrpcCompositeCodec) context.getHttpMessageDecoder();
- MethodMetadata methodMetadata =
context.getMethodMetadata();
-
grpcCompositeCodec.setDecodeTypes(methodMetadata.getActualRequestTypes());
- grpcCompositeCodec.setEncodeTypes(new Class[]
{methodMetadata.getActualResponseType()});
+ grpcCompositeCodec.loadPackableMethod(methodDescriptor);
getServerChannelObserver().setResponseEncoder(grpcCompositeCodec);
+ rawMessage = new ByteArrayInputStream(data);
}
- merged.write(data);
- getHttpMessageListener().onMessage(new
ByteArrayInputStream(merged.toByteArray()));
+ getStreamingDecoder().invokeListener(rawMessage);
} catch (IOException e) {
throw new DecodeException(e);
}
Review Comment:
**Performance Issue**: The method 'onFragmentMessage' performs a rawMessage
read operation inside a try block without checking the size of the InputStream,
which could lead to inefficient memory usage and potential performance
degradation if the InputStream is significantly large. <br> **Fix**: Implement
a mechanism to check the size of the InputStream before reading it. If the size
exceeds a certain threshold, consider processing the stream in chunks or using
a more efficient way to handle large streams to optimize memory usage and
performance. <br> **Code Suggestion**:
```
Implement a mechanism to check the size of the InputStream before reading
it. If the size exceeds a certain threshold, consider processing the stream in
chunks or using a more efficient way to handle large streams to optimize memory
usage and performance.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
