Inefficient Usage of Java Collection

[FastAtlas]

Hi,

We find that there are several ArrayList objects which are not manipulated by random access. Due to the memory reallocation triggered in the successive insertions, the time complexity of add method of ArrayList is amortized O(1). We notice that these objects are only used for traversal, the retrieval, and removal of the first or the last element.

This functionality can be implemented by LinkedList. Moreover, the insertion of LinkedList is strictly O(1) time complexity because no memory reallocation occurs.

Meanwhile, we also found several LinkedHashSet and LinkedHashMap objects which are not necessary to maintain the order of insertions. To achieve the same functionality, HashHap and HashSet are enough. The replacement can also reduce the time cost in the modification of the map objects.

We discovered the above inefficient usage of containers by our tool Ditto. The patch is submitted. Could you please check and accept it? We have tested the patch on our PC. The patched program works well.

Bests

Ditto

[sofastack-bot]

Hi @DittoTool, welcome to SOFAStack community, Please sign Contributor License Agreement!

After you signed CLA, we will automatically sync the status of this pull request in 3 minutes.

[codecov]

Codecov Report

Merging #1079 (522b9c8) into master (5f3dd7e) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master    #1079      +/-   ##
============================================
- Coverage     68.93%   68.93%   -0.01%     
- Complexity      825      826       +1     
============================================
  Files           409      409              
  Lines         17693    17693              
  Branches       2744     2744              
============================================
- Hits          12197    12196       -1     
- Misses         4079     4081       +2     
+ Partials       1417     1416       -1     

Impacted Files	Coverage Δ
...ay/sofa/rpc/client/AllConnectConnectionHolder.java	60.51% <0.00%> (-0.26%)	⬇️
...ofa/rpc/codec/sofahessian/BlackListFileLoader.java	77.55% <100.00%> (ø)
...n/java/com/alipay/sofa/rpc/client/RouterChain.java	89.87% <100.00%> (ø)
...n/java/com/alipay/sofa/rpc/filter/FilterChain.java	83.15% <100.00%> (ø)
...m/alipay/sofa/rpc/config/JAXRSProviderManager.java	75.00% <100.00%> (ø)
...ava/com/alipay/sofa/rpc/common/MetadataHolder.java	90.00% <100.00%> (ø)
.../sofa/rpc/tracer/sofatracer/RestTracerAdapter.java	54.83% <100.00%> (ø)
...a/com/alipay/sofa/rpc/common/utils/ClassUtils.java	78.23% <0.00%> (-2.05%)	⬇️
...lipay/sofa/rpc/registry/consul/ConsulRegistry.java	47.80% <0.00%> (+0.54%)	⬆️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f3dd7e...522b9c8. Read the comment docs.

[OrezzerO]

Thank you for your PR, we will check the changes later.

[leyou240]

@DittoTool Where can I get the Ditto tool?

[FastAtlas]

@DittoTool Where can I get the Ditto tool?

Currently, it is a commercial tool. It might be open-sourced later.:)

[OrezzerO]

LGTM

[FastAtlas]

LGTM

Thank you for confirmation :)

[leizhiyuan]

please see quarkusio/quarkus#19962 (comment)

I won't comment on the other changes, but I'd like to highlight one thing about the ArrayList -> LinkedList move.

The advantages of linked lists over arrays in scenarios where the list is only iterated or appended are only real under the RAM computation model. Real computers are not RAM computers, they employ memory hierarchies, and so the same logic does not apply. Using a LinkedList is almost always worse than using an ArrayList.

[FastAtlas]

please see quarkusio/quarkus#19962 (comment)

I won't comment on the other changes, but I'd like to highlight one thing about the ArrayList -> LinkedList move.

The advantages of linked lists over arrays in scenarios where the list is only iterated or appended are only real under the RAM computation model. Real computers are not RAM computers, they employ memory hierarchies, and so the same logic does not apply. Using a LinkedList is almost always worse than using an ArrayList.

I think there exists a gap between the design and implementation of these collections. The new version of JVM might introduce some optimizations upon ArrayList. However, from the complexity perspective, LinkedList is still better than ArrayList in some cases. Otherwise, why LinkedList still exists in JCF?

Ditto receives the complexity specification as its input and is unaware of the JVM optimization. On the one hand, the result only suggests the potential inefficient usage. Also, dynamic profiling statistics can be introduced to make the recommendation more precise. On the other hand, we just take the complexity specification as our assumption. If we adopt the implementation in the real JVM, we can also recommend replacing the LinkedList objects with ArrayList objects in the program.

Of course, such changes might not make big difference. The project sofa-rpc does not have serve cases of inefficient container usage. In the patch, we only mention three kinds of transformations: ArrayList->LinkedList, LinkedHashMap->HashMap, and LinkedHashSet->HashSet. The three patterns actually have little influence on complexity. However, in other cases, for example, a list can be used as a set, so Ditto will recommend replacing a list object with a HashSet object. The contains methods have totally different complexities in list and set.

[EvenLjj]

LGTM