Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Inefficient Usages of Java Collections #1125

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Inefficient Usages of Java Collections #1125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9462974
Transform LinkedHashMap to HashMap
FastAtlas Sep 14, 2021
f9b07bb
Transform TreeSet to HashSet
FastAtlas Sep 14, 2021
0ad4781
Update GlowHelpMap.java
FastAtlas Sep 14, 2021
3f8d177
Transform ArrayList to HashSet
FastAtlas Sep 14, 2021
52e524f
Transform two ArrayList to HashSet
FastAtlas Sep 14, 2021
e5ec61f
Transform six ArrayList objs to LinkedList objs
FastAtlas Sep 14, 2021
fbfb06b
Update GlowHelpMap.java
FastAtlas Sep 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/main/java/net/glowstone/GlowPluginTypeDetector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,20 @@
import java.io.IOException;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.LinkedList;
import java.util.Enumeration;
import java.util.List;
import java.util.zip.ZipEntry;
import java.util.zip.ZipFile;

public class GlowPluginTypeDetector {

public List<File> bukkitPlugins = new ArrayList<>();
public List<File> spongePlugins = new ArrayList<>();
public List<File> canaryPlugins = new ArrayList<>();
public List<File> forgefPlugins = new ArrayList<>();
public List<File> forgenPlugins = new ArrayList<>();
public List<File> unrecognizedPlugins = new ArrayList<>();
public List<File> bukkitPlugins = new LinkedList<>();
public List<File> spongePlugins = new LinkedList<>();
public List<File> canaryPlugins = new LinkedList<>();
public List<File> forgefPlugins = new LinkedList<>();
public List<File> forgenPlugins = new LinkedList<>();
public List<File> unrecognizedPlugins = new LinkedList<>();

private File directory;

Expand Down
10 changes: 7 additions & 3 deletions src/main/java/net/glowstone/generator/objects/IceSpike.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import org.bukkit.World;
import org.bukkit.block.Block;

import java.util.Set;
import java.util.HashSet;
import java.util.Arrays;
import java.util.Random;

Expand All @@ -25,6 +27,8 @@ public boolean generate(World world, Random random, int sourceX, int sourceY, in
}
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?


for (int x = -stemRadius; x <= stemRadius; x++) {
for (int z = -stemRadius; z <= stemRadius; z++) {
int stackHeight = MAX_STEM_HEIGHT;
Expand All @@ -33,7 +37,7 @@ public boolean generate(World world, Random random, int sourceX, int sourceY, in
}
for (int y = tipOffset - 1; y >= -3; y--) {
Block block = world.getBlockAt(sourceX + x, sourceY + y, sourceZ + z);
if (Arrays.asList(MATERIALS).contains(block.getType())
if (materialSet.contains(block.getType())
|| block.getType() == Material.PACKED_ICE) {
block.setType(Material.PACKED_ICE);
stackHeight--;
Expand Down Expand Up @@ -63,14 +67,14 @@ public boolean generate(World world, Random random, int sourceX, int sourceY, in
// tip shape in top direction
Block block = world
.getBlockAt(sourceX + x, sourceY + tipOffset + y, sourceZ + z);
if (Arrays.asList(MATERIALS).contains(block.getType())) {
if (materialSet.contains(block.getType())) {
block.setType(Material.PACKED_ICE);
succeeded = true;
}
if (radius > 1 && y != 0) { // same shape in bottom direction
block = world
.getBlockAt(sourceX + x, sourceY + tipOffset - y, sourceZ + z);
if (Arrays.asList(MATERIALS).contains(block.getType())) {
if (materialSet.contains(block.getType())) {
block.setType(Material.PACKED_ICE);
succeeded = true;
}
Expand Down
9 changes: 7 additions & 2 deletions src/main/java/net/glowstone/generator/objects/trees/RedwoodTree.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
import org.bukkit.World;

import java.util.Random;
import java.util.HashSet;
import java.util.Set;

public class RedwoodTree extends GenericTree {

Expand Down Expand Up @@ -45,6 +47,7 @@ protected final void setLeavesHeight(int leavesHeight) {

@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.

for (int y = baseY; y <= baseY + 1 + height; y++) {
// Space requirement
int radius; // default radius if above first block
Expand All @@ -59,7 +62,7 @@ public boolean canPlace(int baseX, int baseY, int baseZ, World world) {
if (y >= 0 && y < 256) {
// we can overlap some blocks around
Material type = blockTypeAt(x, y, z, world);
if (!overridables.contains(type)) {
if (!overridableSet.contains(type)) {
return false;
}
} else { // height out of range
Expand All @@ -81,6 +84,8 @@ public boolean generate(World world, Random random, int blockX, int blockY, int
int radius = random.nextInt(2);
int peakRadius = 1;
int minRadius = 0;
Set<Material> overridableSet = new HashSet<>(overridables);

for (int y = blockY + height; y >= blockY + leavesHeight; y--) {
// leaves are built from top to bottom
for (int x = blockX - radius; x <= blockX + radius; x++) {
Expand All @@ -107,7 +112,7 @@ && blockTypeAt(x, y, z, world) == Material.AIR) {
// generate the trunk
for (int y = 0; y < height - random.nextInt(3); y++) {
Material type = blockTypeAt(blockX, blockY + y, blockZ, world);
if (overridables.contains(type)) {
if (overridableSet.contains(type)) {
delegate.setType(world, blockX, blockY + y,
blockZ, logType);
}
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/net/glowstone/util/GlowHelpMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
mastercoms marked this conversation as resolved.
Show resolved Hide resolved

/**
Expand Down Expand Up @@ -59,7 +58,7 @@ public final class GlowHelpMap implements HelpMap {
*/
public GlowHelpMap(GlowServer server) {
this.server = server;
helpTopics = new TreeMap<>(NAME_COMPARE);
helpTopics = new HashMap<>();
Copy link
Member

@mastercoms mastercoms Sep 15, 2021 •
edited
Loading

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.

defaultTopic
= new IndexHelpTopic("Index", null, null, indexTopics, "Use /help [n] to get page"
+ " n of help.");
Expand Down
7 changes: 3 additions & 4 deletions src/main/java/net/glowstone/util/library/Library.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import org.apache.maven.artifact.versioning.ComparableVersion;
import org.jetbrains.annotations.NonNls;

import java.util.LinkedHashMap;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;

Expand Down Expand Up @@ -157,8 +157,7 @@ public static Library fromConfigMap(Map<?, ?> configMap) {
* @return A map that is able to be serialized into a config.
*/
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.

configMap.put(GROUP_ID_KEY, libraryKey.getGroupId());
configMap.put(ARTIFACT_ID_KEY, libraryKey.getArtifactId());
configMap.put(VERSION_KEY, version);
Expand All @@ -168,7 +167,7 @@ public static Library fromConfigMap(Map<?, ?> configMap) {
}

if (checksumType != null && checksumValue != null) {
Map<String, Object> checksumMap = new LinkedHashMap<>();
Map<String, Object> checksumMap = new HashMap<>();
checksumMap.put(CHECKSUM_TYPE_KEY, checksumType.getName());
checksumMap.put(CHECKSUM_VALUE_KEY, checksumValue);
configMap.put(CHECKSUM_KEY, checksumMap);
Expand Down