Skip to content

Conversation

@FastAtlas
Copy link

Hi,

We find that there are several inefficient usages of Java Collections:

  1. The contains method is invoked upon a list object. We recommend replacing it with a HashSet.
  2. There is no iteration occurring upon a TreeMap and LinkedHashMap, thus the insertion order does not matter. We recommend replacing it with a HashMap.
  3. ArrayList is inserted before an iteration, while multiple memory reallocation might occur when the size of the list exceeds its capacity. We recommend replacing it with a LinkedList.

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

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

public GlowHelpMap(GlowServer server) {
this.server = server;
helpTopics = new TreeMap<>(NAME_COMPARE);
helpTopics = new HashMap<>();
Copy link
Member

@mastercoms mastercoms Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is iterated upon in Bukkit (a Maven dependency of ours, see net.glowstone.glowkit), in HelpCommand.

public Map<?, ?> toConfigMap() {
// Using LinkedHashMap to keep the props in order when written into the config file.
Map<String, Object> configMap = new LinkedHashMap<>();
Map<String, Object> configMap = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't called within Glowstone (yet?), but the interface guarantees ordering for a config file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Our tool finds that the order is actually not used because the map object is not traversed. Of course, you can maintain it as a LinkedHashMap if you want to assure the extensibility of the program. For the current version, the HashMap is a better choice.

}
boolean succeeded = false;
int stemRadius = Math.max(0, Math.min(MAX_STEM_RADIUS, tipRadius - 1));
Set<Material> materialSet = new HashSet<>(Arrays.asList(MATERIALS));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would much rather have the MATERIALS variable be directly converted to a HashSet.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I agree with you.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnumSet instead of HashSet?


@Override
public boolean canPlace(int baseX, int baseY, int baseZ, World world) {
Set<Material> overridableSet = new HashSet<>(overridables);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should convert the base class' overridables to a HashSet, to prevent allocations every call.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can redesign the class to achieve better optimization. Currently, such allocation also improves the efficiency of the single function indeed.

@mastercoms
Copy link
Member

Also, can the CLA be signed by @DittoTool?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants