BitoAgent commented on code in PR #13786:
URL: https://github.com/apache/dubbo/pull/13786#discussion_r1575352196
##########
dubbo-common/src/main/java/org/apache/dubbo/config/AbstractInterfaceConfig.java:
##########
@@ -252,7 +252,7 @@ protected void postProcessAfterScopeModelChanged(ScopeModel
oldScopeModel, Scope
}
if (CollectionUtils.isNotEmpty(this.registries)) {
this.registries.forEach(registryConfig -> {
- if (registryConfig.getScopeModel() != applicationModel) {
+ if (registryConfig != null && registryConfig.getScopeModel()
!= applicationModel) {
Review Comment:
**Issue**: Adding null check for registryConfig to prevent potential
NullPointerException. <br> **Fix**: Add a null check before accessing
registryConfig properties. <br> **Code Suggestion**:
```
- if (registryConfig.getScopeModel() != applicationModel) {
+ if (registryConfig != null &&
registryConfig.getScopeModel() != applicationModel) {
```
##########
dubbo-remoting/dubbo-remoting-http12/src/main/java/org/apache/dubbo/remoting/http12/message/LengthFieldStreamingDecoder.java:
##########
@@ -130,16 +127,12 @@ private void deliver() {
}
private void processHeader() throws IOException {
- ByteArrayOutputStream bos = new
ByteArrayOutputStream(lengthFieldOffset + lengthFieldLength);
byte[] offsetData = new byte[lengthFieldOffset];
int ignore = accumulate.read(offsetData);
- bos.write(offsetData);
processOffset(new ByteArrayInputStream(offsetData), lengthFieldOffset);
byte[] lengthBytes = new byte[lengthFieldLength];
ignore = accumulate.read(lengthBytes);
- bos.write(lengthBytes);
requiredLength = bytesToInt(lengthBytes);
- this.dataHeader = new ByteArrayInputStream(bos.toByteArray());
// Continue reading the frame body.
state = DecodeState.PAYLOAD;
Review Comment:
**Performance Issue**: The method processHeader inefficiently processes
header data by reading into byte arrays without considering the impact on
memory and CPU usage, especially in high-throughput scenarios. <br> **Fix**:
Optimize the processHeader method to minimize memory allocations and improve
CPU efficiency. Consider using a buffer pool to reuse byte arrays and reduce
garbage collection pressure. <br> **Code Suggestion**:
```
- byte[] offsetData = new byte[lengthFieldOffset];
- int ignore = accumulate.read(offsetData);
- processOffset(new ByteArrayInputStream(offsetData),
lengthFieldOffset);
- byte[] lengthBytes = new byte[lengthFieldLength];
- ignore = accumulate.read(lengthBytes);
- requiredLength = bytesToInt(lengthBytes);
+ ByteBuffer buffer = ByteBuffer.allocate(lengthFieldOffset +
lengthFieldLength);
+ accumulate.read(buffer.array(), 0, lengthFieldOffset +
lengthFieldLength);
+ processOffset(new ByteArrayInputStream(buffer.array(), 0,
lengthFieldOffset), lengthFieldOffset);
+ requiredLength = bytesToInt(Arrays.copyOfRange(buffer.array(),
lengthFieldOffset, lengthFieldOffset + lengthFieldLength));
```
##########
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:
**Scalability Issue**: The implementation of loading PackableMethod through
a ConcurrentHashMap may not be fully optimized for concurrent access patterns,
potentially leading to scalability issues under high load. <br> **Fix**:
Consider using a more efficient concurrent data structure or optimizing the
current ConcurrentHashMap usage to reduce lock contention and improve
scalability. <br> **Code Suggestion**:
```
-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));
+ConcurrentHashMap<MethodDescriptor, PackableMethod> cacheMap =
(ConcurrentHashMap<MethodDescriptor, PackableMethod>) url.getServiceModel()
+ .getServiceMetadata()
+ .getAttributeMap()
+ .computeIfAbsent(PACKABLE_METHOD_CACHE, k -> new
ConcurrentHashMap<>(16, 0.9f, 1));
+packableMethod = cacheMap.computeIfAbsent(methodDescriptor, md ->
PackableMethodFactory.createOptimized(methodDescriptor, url, mediaType));
```
--
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]
