Use hash map to store settler images instead of 4-dim array #733

ewaldbenes · 2018-06-15T22:26:47Z

No description provided.

andreas-eberle · 2018-07-18T17:48:00Z

jsettlers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMap.java

-	private SettlerImageMapItem getMapItem(EMovableType movableType, EMovableAction action, EMaterialType material, EDirection direction) {
-		SettlerImageMapItem item = this.map[movableType.ordinal()][action.ordinal()][material.ordinal][direction.ordinal];
+	private SettlerImageMapItem getMapItem(SettlerImageFlavor settlerImageFlavor) {
+		SettlerImageMapItem item = map.get(settlerImageFlavor);


Did you time the old and the new retrieval? I guess the performance of accessing a hash map vs. accessing a 4 dim array might be worse. How much is it for a larger number of calls (e.g. 1000 or so)?

Actually, accessing a hash map should be faster than a 4 dimensional array (fewer memory accesses / cache misses).

Access of hash map by key is 0(1) so the same as the existing index lookup.

Memory footprint is far less because the array was as big as all possible combinations. I measured over a million. By using a hash map it went down to a little more than 1000. The expressiveness and the lower memory footprint were the 2 major reason why I did this refactoring.

Actually the HashMap implementation of java has no guaranteed access of O(1) but a worst case access of O(n) instead ;-). But we don't care about worst case complexity here, we care about actual performance. Measuring this is hard - you cannot measure the GC delays you get in a few frames so easily. So you need to track speed and GC consumption with both the old and new methods in a reproducible setting (should be enough to test it on PC though, Android is even more complex for this).

You said that the HashMap should be faster than the array. I'll look again at the hash function inside the flavor class and take care about object creation inside drawing methods to avoid GC pressure. Your two comments seem to contradict?

The array removal was merely a read- and maintainability change plus a huge memory gain.

andreas-eberle · 2018-07-18T17:49:55Z

jsettlers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMap.java


+	public void loadFromMoveablesTextFile() {


typo "Movables" instead of "Mov[e]ables"

andreas-eberle · 2018-07-18T17:51:34Z

jsettlers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMap.java

-	}
-
-	private void readFromFile(InputStream file, int[][][][] priorities) throws IOException {
+	private void readFromStream(InputStream file) throws IOException {


This method can be inlined. Both, the caller and this method, are now two-liners.

andreas-eberle · 2018-07-18T17:52:33Z

jsettlers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMap.java

@@ -182,18 +168,18 @@ private EDirection parseDirection(final String directionString) {
 		return direction;
 	}

-	private int calculatePriority(EMovableType type, EMovableAction action, EMaterialType material, EDirection direction) {
+	private int calculatePriority(SettlerImageFlavor settlerImageFlavor) {


Move this method to SettlerImageFlavor, since it only accesses fields of the passed object.

andreas-eberle · 2018-07-18T17:53:39Z

...ers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageFlavor.java

+public final class SettlerImageFlavor {
+
+    public static final SettlerImageFlavor NONE = new SettlerImageFlavor(null, null, null, null);
+    private final EMovableType type;


Use tabs as indents.

andreas-eberle · 2018-07-18T17:54:22Z

jsettlers.graphics/src/main/java/jsettlers/graphics/map/draw/ImageProvider.java

@@ -51,7 +53,8 @@
 *
 * @author michael
 */
-public final class ImageProvider {
+// TODO: removed final for testing. introduce it again when some supertype could be extracted


Remove the todo. The final isn't needed.

andreas-eberle · 2018-07-18T17:55:47Z

...rs.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMapItem.java

+		return Objects.hash(file, sequenceIndex, start, duration);
+	}
+
+    public int imageIndex(float progress) {


Use tabs as indent.

andreas-eberle · 2018-07-18T18:15:06Z

I saw that the jsettlers.graphics:test wasn't executed by travis and added this.

andreas-eberle · 2018-07-18T18:15:32Z

...rs.graphics/src/test/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMapTest.java

@@ -0,0 +1,73 @@
+package jsettlers.graphics.map.draw.settlerimages;


Please add the copyright notice.

andreas-eberle · 2018-07-18T18:15:57Z

...rs.graphics/src/test/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMapTest.java

+public class SettlerImageMapTest {
+
+    private static final SettlerImageFlavor TEST_IMAGE_FLAVOR = new SettlerImageFlavor(
+            EMovableType.BEARER,


Use tabs and use one line here only. We have a line length of 200 characters.

andreas-eberle · 2018-07-18T18:17:52Z

...rs.graphics/src/test/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMapTest.java

+    }
+
+    @Test
+    public void givenTextWithTestEntry_whenLoad_thenContainsTestEntry() throws IOException {


Please don't use _ in method names. Java convention expects camel case only.

michaelzangl · 2018-07-18T19:07:56Z

...ics/src/main/java/jsettlers/graphics/map/controls/original/panel/selection/SelectionRow.java

-						EMovableAction.NO_ACTION, EMaterialType.NO_MATERIAL,
-						EDirection.SOUTH_EAST, 0);
+		Image image = SettlerImageMap.getInstance().getImageForSettler(
+				new SettlerImageFlavor(type, EMovableAction.NO_ACTION, EMaterialType.NO_MATERIAL, EDirection.SOUTH_EAST),


This is the main problem I see with your approach (and the reason why we did it the old way):

You are creating an object here that the JIT cannot (or at least in my tests with Android could not) inline. Mind that Android seems to be far worse at optimizing code than the normal Java JIT is.
This will make the young generation grow and thus create a lot of GC activity, since you might have thousands of settlers on one screen, especially when zooming out.
This is the reason why the drawing code attempts not to create any objects during painting phases.

We should test this and - if it is a problem - use a long instead of the SettlerImageFlavor (you can then create a static method that generates that unique long from the given parameters). Then use a library that supports long hash maps.

You're right. I have overseen that fact. Maybe I can find a way to cache the SettlerImageFlavor instead of using a long since it would be the same approach. What do you think?

The usual way to cache such objects is to put them in a hash table - You have a chicken and egg problem then ;-).

Generating the long is no big deal and should be pretty fast. A long -> Object hash map is probably the fastest you can get (in this case probably ~10 times faster than the way you are using now - at least that is my guess from my experience, might be more or less).

I've looked at this again. The SettlerImageFlavor can be created once in the constructor and the overhead is simply gone.

michaelzangl · 2018-07-18T19:08:24Z

jsettlers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMap.java


-	private final SettlerImageMapItem[][][][] map;
+	final HashMap<SettlerImageFlavor, SettlerImageMapItem> map = new HashMap<>();


It's used to assert contents in the map in the class test.

michaelzangl · 2018-07-18T19:13:10Z

...rs.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMapItem.java

+
+	@Override
+	public int hashCode() {
+		return Objects.hash(file, sequenceIndex, start, duration);


Same allocation problem here - use direct hash code computation instead to avoid allocating an array.

This object is used as a value in the hash map so no hashing is done. I don't know why I added hashCode and equals here ^^

michaelzangl · 2018-07-18T19:44:05Z

...ers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageFlavor.java

+
+    @Override
+    public int hashCode() {
+        return Objects.hash(type, action, material, direction);


@ewaldbenes Then my comment should have been placed here - same problem, object allocation in hashCode().

Now I understand. I will take care of object allocations hashing and drawing methods.

…emory. Add unit tests for settler map.

Convert indents from space to tabs. Fix typos.

…nstance constant.

andreas-eberle · 2018-09-10T20:27:19Z

@michaelzangl: Can you have a look at the updated PR?

michaelzangl

Looks fine now. Code quality has improved a lot compared to the tings I hacked...

I would be really interested in speed impacts, but I estimate it to be not so much of an issue and that the performance should be about the same.

michaelzangl · 2018-09-10T20:33:18Z

jsettlers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageMap.java

-		addEntryToMap(priorities, type, action, material, direction, new SettlerImageMapItem(fileIndex, sequence, start, duration), priority);
+		SettlerImageFlavor flavor = new SettlerImageFlavor(type, action, material, direction);
+		int priority = flavor.calculatePriority();
+		if(!priorities.containsKey(flavor) || priorities.get(flavor) < priority) {


You can make this faster by using:

var value = priorities.get(flavor); if (value != null || value ...) {}

michaelzangl · 2018-09-10T20:36:39Z

...ers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageFlavor.java

+	private final EDirection direction;
+
+	public SettlerImageFlavor(EMovableType type, EMovableAction action, EMaterialType material, EDirection direction) {
+		this.type = type;


You can ensure that the parameters are not null by adding Objects.requireNonNull() calls.

The map contains one default entry that is defined by all parameters as null and comes from "movables.txt" file, entry ",,,=10, 0, 0, 1". * is converted to null. That's why I cannot prohibit null.

michaelzangl · 2018-09-10T20:39:13Z

...ers.graphics/src/main/java/jsettlers/graphics/map/draw/settlerimages/SettlerImageFlavor.java

+	@Override
+	public int hashCode() {
+		int result = type != null ? type.hashCode() : 0;
+		result = 31 * result + (action != null ? action.hashCode() : 0);


Same here - java is very efficient at testing for non-null and throwing an exception if a value is null.

andreas-eberle reviewed Jul 18, 2018

View reviewed changes

michaelzangl reviewed Jul 18, 2018

View reviewed changes

ewaldbenes and others added 4 commits August 1, 2018 16:07

Use hash map to store settler images instead of 4-dim array to save m…

995ec0c

…emory. Add unit tests for settler map.

Change Gradle testCompile to testImplementation.

c968349

Change Gradle testCompile to testImplementation.

9d16d74

Let travis run :jsettlers.graphics:test

ae03171

ewaldbenes force-pushed the refactor/settler-image-map branch from a91311e to ae03171 Compare August 1, 2018 14:10

ewaldbenes added 2 commits August 1, 2018 17:17

Avoid array allocation in hashCode implementations.

450bb54

Convert indents from space to tabs. Fix typos.

Move movable image retrieval out of draw method because it is a per-i…

2f15ae5

…nstance constant.

michaelzangl approved these changes Sep 10, 2018

View reviewed changes

Replace Map.contains and Map.get with Map.get and null check

1a726f5

ewaldbenes force-pushed the refactor/settler-image-map branch from 4d10390 to 1a726f5 Compare September 11, 2018 10:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use hash map to store settler images instead of 4-dim array #733

Use hash map to store settler images instead of 4-dim array #733

ewaldbenes commented Jun 15, 2018

andreas-eberle Jul 18, 2018

michaelzangl Jul 18, 2018

ewaldbenes Jul 18, 2018

michaelzangl Jul 18, 2018

ewaldbenes Aug 1, 2018

andreas-eberle Jul 18, 2018

andreas-eberle Jul 18, 2018

andreas-eberle Jul 18, 2018

andreas-eberle Jul 18, 2018

andreas-eberle Jul 18, 2018

andreas-eberle Jul 18, 2018

andreas-eberle commented Jul 18, 2018

andreas-eberle Jul 18, 2018

andreas-eberle Jul 18, 2018

andreas-eberle Jul 18, 2018

michaelzangl Jul 18, 2018

ewaldbenes Jul 18, 2018

michaelzangl Jul 18, 2018

ewaldbenes Aug 1, 2018

michaelzangl Jul 18, 2018

ewaldbenes Aug 1, 2018

michaelzangl Jul 18, 2018

ewaldbenes Jul 18, 2018

michaelzangl Jul 18, 2018

ewaldbenes Aug 1, 2018

andreas-eberle commented Sep 10, 2018

michaelzangl left a comment

michaelzangl Sep 10, 2018

michaelzangl Sep 10, 2018

ewaldbenes Sep 11, 2018

michaelzangl Sep 10, 2018

		@@ -0,0 +1,73 @@
		package jsettlers.graphics.map.draw.settlerimages;


		private final SettlerImageMapItem[][][][] map;
		final HashMap<SettlerImageFlavor, SettlerImageMapItem> map = new HashMap<>();

Use hash map to store settler images instead of 4-dim array #733

Are you sure you want to change the base?

Use hash map to store settler images instead of 4-dim array #733

Conversation

ewaldbenes commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreas-eberle commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreas-eberle commented Sep 10, 2018

michaelzangl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment