Optimising CubeWatchers unloading #365

Foghrye4 · 2018-04-22T18:15:21Z

Working marvelously.

Barteks2x

Looks good overall, just a few things to improve

Barteks2x · 2018-04-22T18:17:59Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+            public T next() {
+                prev = next;
+                next = null;
+                return prev;


this assumes hasNext has been called. Not a problem for most use cases but could be susprising to discover that if (!thing.isEmpty()) { return thing.iterator().next(); } doesn't work.

Barteks2x · 2018-04-22T18:19:06Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+    /** Contain all data in sorting order */
+    private final List<T> data = new ArrayList<T>();
+    /** Contain all data without certain order. Used to detect if element is already added */
+    private final Set<T> data2 = new HashSet<T>();


rename to dataAsSet

Barteks2x · 2018-04-22T18:22:57Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+    /** Sort and remove scheduled elements */
+    @SuppressWarnings({"unchecked", "rawtypes"})
+    public void tick() {
+        T[] a = (T[]) data.toArray();


Consider using 2 ObjectArrayLists from fastutils, and swapping them around. FastUtil exposes the internal array from it's array list.

Barteks2x · 2018-04-22T18:25:40Z

All checks failed because MinecraftForge/ForgeGradle#499 (https://travis-ci.org/OpenCubicChunks/CubicChunks/builds/369815810?utm_source=github_status&utm_medium=notification)

Barteks2x · 2018-04-22T18:36:32Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+import java.util.function.Predicate;
+
+/**
+ * Helper class to delay removing of an elements to sorting tick. Wrap both ArrayList and HashSet classes inside of it.


Explain why in the comment

Foghrye4 · 2018-04-24T17:51:14Z

I decide to use a raw array of objects as data container. This way we can remove elements right during iterations.

Barteks2x · 2018-04-24T20:11:00Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+        Arrays.sort((T[])data, 0, size, order);
+    }
+
+    /** Check if list is empty. Even when it return true list still could contain 


What is that weird javadoc formatting?

Barteks2x · 2018-04-24T20:12:07Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+        }
+        Object old = data[index];
+        data[index] = element;
+        this.add((T) old);


is that behavior actually useful in any way? It's likely nothing actually needs to insert at specific position in the array.

This function used here:

CubicChunks/src/main/java/cubicchunks/server/PlayerCubeMap.java

Line 370 in 753ef1c

cubesToGenerate.add(0, playerInstance);

To add cube to generation query with high priority.

And your code completely missed the point as when a second none is added the first one will be moved. To the end. Just make the values stored internally in reverse order so you can have insertFirst method and it would only need to append it at the end of the array.

Barteks2x · 2018-04-24T20:13:21Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+    }
+
+    private void grow() {
+        data = Arrays.copyOf(data, data.length << 1);


I generally don't have an issue wit bitshifts when they are actually useful but here it only obfuscates the code for people who don't know much about them

Foghrye4 · 2018-04-25T17:42:30Z

Considering adding elements to the start of a list I have even better idea.

Barteks2x

Just a few minor corrections, and when you are done, run profiler/sampler again and show the results.,

Barteks2x · 2018-04-25T17:45:03Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+     * @return if list contain any accessible data
+     */
+    public boolean isEmpty() {
+        return size - toRemove.size() == 0;


You may want to add assert size - toRemove.size() >= 0; (my genIntellijRuns task enables assertions by default)

Barteks2x · 2018-04-25T17:47:35Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+
+            private void peekNext() {
+                while (next == null && a < start + size) {
+                    T e = (T) data[++a - 1];


either split it into 2 lines or use increment properly and do data[i++]

Barteks2x · 2018-04-25T17:48:11Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+                    T e = (T) data[++a - 1];
+                    if (toRemove.remove(e)) {
+                        dataAsSet.remove(e);
+                        data[--a] = data[start + --size];


There you should definitely split it into more lines. Just because it works doesn't mean it's a good idea.

Barteks2x · 2018-04-25T17:48:26Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+            @Override
+            public void remove() {
+                dataAsSet.remove(prev);
+                data[--a] = data[start + --size];


Barteks2x · 2018-04-25T17:48:40Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+            if (predicate.test(a)) {
+                toRemove.remove(a);
+                dataAsSet.remove(a);
+                data[i] = data[start + --size];


Barteks2x · 2018-04-25T17:48:52Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+            throw new NullPointerException("This list does not allow null elements.");
+        if (start <= 0)
+            grow();
+        data[--start] = element;


Barteks2x · 2018-04-25T17:49:56Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+     * to remove non-existing elements will cause nasty bugs.
+     */
+    public void remove(T entry) {
+        toRemove.add(entry);


how does it handle removing element that doesn't exist in the list? I'm pretty sure it breaks size and isEmpty. If you don't want to handle such things, at least crash in those cases (PlayerCubeMap shouldn't be doing that). Same for other strange cases like adding the same thing twice.

Foghrye4 · 2018-04-25T18:22:11Z

Done. No noticeable anything. Snapshot created after flying straight forward in creative mode for a few minutes.

Barteks2x · 2018-04-25T18:24:10Z

This is running with vanilla world, not cubic chunks world.

Foghrye4 · 2018-04-25T18:38:56Z

Ehm...

Foghrye4 · 2018-04-27T18:14:56Z

Ok, now its finally working fine.

(sampling only cubicchunks.server. package)

Barteks2x · 2018-04-27T19:06:54Z

This seems to actually be either faster to sort or slower overall than the first implementation. Can you actually measure the time it takes to run the methods on average with the first and the current version? (git reflog should still show those old commits that you overwrote, unless you ran git gc)

Barteks2x · 2018-04-27T19:13:53Z

src/main/java/cubicchunks/util/WatchersSortingList.java

+                while (next == null && i < start + size) {
+                    T e = (T) data[i++];
+                    if (toRemove.remove(e)) {
+                        remove();


If it actually is slower, this is likely the part responsible for it. Just do contains() check and skip it here and do mass-removal on tick. The System.arrayCopy during removal of every single element is what this PR was supposed to avoid.

Foghrye4 · 2018-04-28T17:28:02Z

I made a one simple trick to use sorting algorithm in getting rid of dead elements.
Here is a results of JMH benchmarking:

Benchmark                Mode  Cnt      Score      Error  Units
MyBenchmark.testNew  thrpt   20  63730.222 ± 5343.953  ops/s
MyBenchmark.testOld  thrpt   20    771.625 ±  543.384  ops/s

"Old" - is a yesterday version of a list.
"New" - is current version of a list.
Here is a benchmark code: https://pastebin.com/7PnNZTNJ
Here is a VisualVM results:

Barteks2x · 2018-04-28T17:52:52Z

This just tests the lists, not the whole PlayerCubeMap.tick() which is what actually counts. A lot of time is spent in hasNext, which calls peekNext which removed any elements in the toRemove set, one by one. With System.arrayCopy each time. And it is an issue because sort() isn't done every tick, you should probably just skip the al;ready removed elements during iteration.

Foghrye4 · 2018-04-28T18:09:54Z

Done as requested.

Barteks2x · 2018-04-28T18:16:25Z

And update the sampler/profiler screenshot

Foghrye4 · 2018-04-28T18:16:44Z

Sampling results:

Barteks2x · 2018-04-28T18:20:43Z

As you can see, hasNext takes significantly less time than before. Merging it now.

Barteks2x reviewed Apr 22, 2018

View reviewed changes

Foghrye4 force-pushed the remove_cubewatcher_fix branch from bcb306e to 57f60bc Compare April 24, 2018 17:50

Foghrye4 force-pushed the remove_cubewatcher_fix branch 2 times, most recently from 2c8eba0 to 753ef1c Compare April 24, 2018 18:38

Barteks2x reviewed Apr 24, 2018

View reviewed changes

Foghrye4 force-pushed the remove_cubewatcher_fix branch from 753ef1c to 9b7ace3 Compare April 25, 2018 17:40

Foghrye4 force-pushed the remove_cubewatcher_fix branch from 9b7ace3 to 9b3cc10 Compare April 25, 2018 17:45

Barteks2x requested changes Apr 25, 2018

View reviewed changes

Foghrye4 force-pushed the remove_cubewatcher_fix branch from 9b3cc10 to 44e2106 Compare April 25, 2018 18:19

Foghrye4 force-pushed the remove_cubewatcher_fix branch 3 times, most recently from 1b741c7 to cecb80e Compare April 27, 2018 18:13

Barteks2x reviewed Apr 27, 2018

View reviewed changes

Foghrye4 force-pushed the remove_cubewatcher_fix branch from cecb80e to 5bb6c7c Compare April 28, 2018 17:16

Optimising CubeWatchers unloading

c53097e

Foghrye4 force-pushed the remove_cubewatcher_fix branch from 5bb6c7c to c53097e Compare April 28, 2018 18:04

Barteks2x approved these changes Apr 28, 2018

View reviewed changes

Barteks2x merged commit 86129f7 into OpenCubicChunks:MC_1.12 Apr 28, 2018

Foghrye4 deleted the remove_cubewatcher_fix branch April 28, 2018 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimising CubeWatchers unloading #365

Optimising CubeWatchers unloading #365

Foghrye4 commented Apr 22, 2018 •

edited

Loading

Barteks2x left a comment

Barteks2x Apr 22, 2018

Barteks2x Apr 22, 2018

Barteks2x Apr 22, 2018

Barteks2x commented Apr 22, 2018 •

edited

Loading

Barteks2x Apr 22, 2018

Foghrye4 commented Apr 24, 2018

Barteks2x Apr 24, 2018

Barteks2x Apr 24, 2018

Foghrye4 Apr 25, 2018

Barteks2x Apr 25, 2018

Barteks2x Apr 24, 2018

Foghrye4 commented Apr 25, 2018

Barteks2x left a comment

Barteks2x Apr 25, 2018

Barteks2x Apr 25, 2018

Barteks2x Apr 25, 2018

Barteks2x Apr 25, 2018

Barteks2x Apr 25, 2018

Barteks2x Apr 25, 2018

Barteks2x Apr 25, 2018

Foghrye4 commented Apr 25, 2018

Barteks2x commented Apr 25, 2018

Foghrye4 commented Apr 25, 2018

Foghrye4 commented Apr 27, 2018

Barteks2x commented Apr 27, 2018 •

edited

Loading

Barteks2x Apr 27, 2018 •

edited

Loading

Foghrye4 commented Apr 28, 2018

Barteks2x commented Apr 28, 2018 •

edited

Loading

Foghrye4 commented Apr 28, 2018

Barteks2x commented Apr 28, 2018

Foghrye4 commented Apr 28, 2018

Barteks2x commented Apr 28, 2018

Optimising CubeWatchers unloading #365

Optimising CubeWatchers unloading #365

Conversation

Foghrye4 commented Apr 22, 2018 • edited Loading

Barteks2x 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

Barteks2x commented Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Foghrye4 commented Apr 24, 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

Foghrye4 commented Apr 25, 2018

Barteks2x 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Foghrye4 commented Apr 25, 2018

Barteks2x commented Apr 25, 2018

Foghrye4 commented Apr 25, 2018

Foghrye4 commented Apr 27, 2018

Barteks2x commented Apr 27, 2018 • edited Loading

Barteks2x Apr 27, 2018 • edited Loading

Choose a reason for hiding this comment

Foghrye4 commented Apr 28, 2018

Barteks2x commented Apr 28, 2018 • edited Loading

Foghrye4 commented Apr 28, 2018

Barteks2x commented Apr 28, 2018

Foghrye4 commented Apr 28, 2018

Barteks2x commented Apr 28, 2018

Foghrye4 commented Apr 22, 2018 •

edited

Loading

Barteks2x commented Apr 22, 2018 •

edited

Loading

Barteks2x commented Apr 27, 2018 •

edited

Loading

Barteks2x Apr 27, 2018 •

edited

Loading

Barteks2x commented Apr 28, 2018 •

edited

Loading