"Cache" SlotMapCache
sameer opened this issue · 8 comments
Hi, see below for an explanation and a patchfile with the code.
Store the slot map cache in a WeakHashMap so that it does not need to be recreated every time.
There will be no memory leak - the object referent will be garbage collected if the ISidedInventory has no other references. This has significantly improved performance on my server.
diff --git a/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java b/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java
index 5b0ccfb..0849a9b 100644
--- a/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java
+++ b/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java
@@ -8,7 +8,7 @@
package logisticspipes.utils;
-import java.util.ArrayList;
+import java.util.*;
import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.inventory.IInventory;
@@ -32,10 +32,13 @@ public final class SidedInventoryMinecraftAdapter implements IInventory {
private final boolean _forExtraction;
private int[] _slotMapCache;
+ private WeakHashMap<ISidedInventory,int[]> slotted = new WeakHashMap<ISidedInventory,int[]>();
+
public SidedInventoryMinecraftAdapter(ISidedInventory sidedInventory, ForgeDirection side, boolean forExtraction) {
_sidedInventory = sidedInventory;
_side = side.ordinal();
_forExtraction = forExtraction;
+ _slotMapCache = slotted.get(sidedInventory);
}
private int[] getSlotMap() {
@@ -55,6 +58,7 @@ public final class SidedInventoryMinecraftAdapter implements IInventory {
}
_slotMapCache = Ints.toArray(list);
}
+ slotted.put(_sidedInventory, _slotMapCache);
}
return _slotMapCache;
}Updated diff with more optimizations
diff --git a/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java b/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java
index 5b0ccfb..f1b3b03 100644
--- a/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java
+++ b/common/logisticspipes/utils/SidedInventoryMinecraftAdapter.java
@@ -8,7 +8,7 @@
package logisticspipes.utils;
-import java.util.ArrayList;
+import java.util.*;
import net.minecraft.entity.player.EntityPlayer;
import net.minecraft.inventory.IInventory;
@@ -30,22 +30,27 @@ public final class SidedInventoryMinecraftAdapter implements IInventory {
public final ISidedInventory _sidedInventory;
private final int _side;
private final boolean _forExtraction;
- private int[] _slotMapCache;
+ private ArrayList<Integer> _slotMapCache;
+
+ private WeakHashMap<ISidedInventory,ArrayList<Integer>> slotted = new WeakHashMap<ISidedInventory,ArrayList<Integer>>();
public SidedInventoryMinecraftAdapter(ISidedInventory sidedInventory, ForgeDirection side, boolean forExtraction) {
_sidedInventory = sidedInventory;
_side = side.ordinal();
_forExtraction = forExtraction;
+ _slotMapCache = slotted.get(sidedInventory);
}
- private int[] getSlotMap() {
+ private ArrayList<Integer> getSlotMap() {
if(_slotMapCache == null) {
if (_side == ForgeDirection.UNKNOWN.ordinal()) {
_slotMapCache = buildAllSidedMap();
} else {
- ArrayList<Integer> list = new ArrayList<Integer>();
int allSlots[] = _sidedInventory.getAccessibleSlotsFromSide(_side);
+
+ ArrayList<Integer> list = new ArrayList<Integer>(allSlots.length);
+
for (int number : allSlots) {
ItemStack item = _sidedInventory.getStackInSlot(number);
if (!list.contains(number) && (!_forExtraction || // check extract condition
@@ -53,17 +58,23 @@ public final class SidedInventoryMinecraftAdapter implements IInventory {
list.add(number);
}
}
- _slotMapCache = Ints.toArray(list);
+ _slotMapCache = list;
}
+ slotted.put(_sidedInventory, _slotMapCache);
}
return _slotMapCache;
}
- private int[] buildAllSidedMap() {
- ArrayList<Integer> list = new ArrayList<Integer>();
-
+ private ArrayList<Integer> buildAllSidedMap() {
+ int[][] slotty = new int[6][];
+ int size = 0;
for (int i = 0; i < 6; i++) {
- int slots[] = _sidedInventory.getAccessibleSlotsFromSide(i);
+ slotty[i] = _sidedInventory.getAccessibleSlotsFromSide(i);
+ if(slotty[i] != null) size += slotty[i].length;
+ }
+ ArrayList<Integer> list = new ArrayList<Integer>(size);
+ for (int i = 0; i < 6; i++) {
+ int[] slots = slotty[i];
for (int number : slots) {
ItemStack item = _sidedInventory.getStackInSlot(number);
if (!list.contains(number) && (!_forExtraction || // check extract condition
@@ -72,32 +83,27 @@ public final class SidedInventoryMinecraftAdapter implements IInventory {
}
}
}
- int slotmap[] = new int[list.size()];
- int count = 0;
- for (int i : list) {
- slotmap[count++] = i;
- }
- return slotmap;
+ return list;
}
@Override
public int getSizeInventory() {
- return getSlotMap().length;
+ return getSlotMap().size();
}
@Override
public ItemStack getStackInSlot(int i) {
- return _sidedInventory.getStackInSlot(getSlotMap()[i]);
+ return _sidedInventory.getStackInSlot(getSlotMap().get(i));
}
@Override
public ItemStack decrStackSize(int i, int j) {
- return _sidedInventory.decrStackSize(getSlotMap()[i], j);
+ return _sidedInventory.decrStackSize(getSlotMap().get(i), j);
}
@Override
public void setInventorySlotContents(int i, ItemStack itemstack) {
- _sidedInventory.setInventorySlotContents(getSlotMap()[i], itemstack);
+ _sidedInventory.setInventorySlotContents(getSlotMap().get(i), itemstack);
}
@Override
@@ -132,7 +138,7 @@ public final class SidedInventoryMinecraftAdapter implements IInventory {
@Override
public ItemStack getStackInSlotOnClosing(int slot) {
- return _sidedInventory.getStackInSlotOnClosing(getSlotMap()[slot]);
+ return _sidedInventory.getStackInSlotOnClosing(getSlotMap().get(slot));
}
@Override
@@ -142,6 +148,6 @@ public final class SidedInventoryMinecraftAdapter implements IInventory {
@Override
public boolean isItemValidForSlot(int slot, ItemStack itemstack) {
- return _sidedInventory.isItemValidForSlot(getSlotMap()[slot], itemstack) && _sidedInventory.canInsertItem(getSlotMap()[slot], itemstack, _side);
+ return _sidedInventory.isItemValidForSlot(getSlotMap().get(slot), itemstack) && _sidedInventory.canInsertItem(getSlotMap().get(slot), itemstack, _side);
}
}
What about all those dynamic implementations that change their accessability? Like ThermalExpansion Machines?
0.o I was not aware that that was possible. This would probably not work in those cases then.
My worry was that though there is a slotmapcache, it is recreated every time the object is instantiated...so it isn't really a cache.
There is of course a way to work around that, but the question is if it is still a performance improvement.
It is a cache because it is not recreated for every slot access.
And for the next time, better create a pull request because this is rather hard to read.
How much performance improvement did you get ?
While this is at 20TPS, it oddly gets a lot worse with many extractor modules in crafting chassis connected to ProjectE condensers. Though this is with the fix, the performance is still a bit poorer than I had hoped it would be. 😕
And concerning the PR sorry about that I should've done it instead of this.
@davboecki how about rebuilding the SlotMapCache upon a onNeighborChange() event wich gest fired towards the Pipe?
Wouldnt that help with Dynamic Implementations?