Opened 6 months ago

Last modified 4 months ago

#2733 new enhancement

I2PSnark: improve sort algorithms

Reported by: Reportage Owned by: zzz
Priority: minor Milestone: undecided
Component: apps/i2psnark Version: 0.9.45
Keywords: I2PSnark, sorting Cc:
Parent Tickets: Sensitive: no

Description (last modified by Reportage)

Patch to provide more intuitive sort for Status and ETA columns. Also requires reversal of sort order for status column in I2PSnarkServlet.java:

//        String sort = ("-2".equals(currentSort)) ? "2" : "-2";
        // show incomplete torrents at top on first click
        String sort = ("2".equals(currentSort)) ? "-2" : "2";
diff --git a/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java b/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
index 56c3115e7..818a4df35 100644
--- a/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
+++ b/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
@@ -200,37 +200,61 @@ class Sorters {
             int rv = getStatus(l) - getStatus(r);
             if (rv != 0)
                 return rv;
-            // use reverse remaining as first tie break
-            return compLong(r.getNeededLength(), l.getNeededLength());
+            else if ((getStatus(l) == 100 && getStatus(r) == 100)||
+                     (getStatus(l) == 98 && getStatus(r) == 98) ||
+                     (getStatus(l) == 40 && getStatus(r) == 40) ||
+                     (getStatus(l) == 10 && getStatus(r) == 10))
+
+                // first tie break by swarm size
+                return compLong(r.getTrackerSeenPeers(), l.getTrackerSeenPeers());
+            else
+                // tie break by active peer count
+                return compLong(r.getPeerCount(), l.getPeerCount());
         }
 
         private static int getStatus(Snark snark) {
-            long remaining = snark.getRemainingLength(); 
+            long remaining = snark.getRemainingLength();
+            int activePeers = snark.getPeerCount();
+            int peers = snark.getTrackerSeenPeers();
+            long downBps = snark.getDownloadRate();
             if (snark.isStopped()) {
+                if (remaining < 0) // magnet
+                    return 50;
+                if (remaining > 0)
+                    return 46;
+                else
+                    return 60;
+            } else {
+                if (downBps > 0)
+                    return 3;
+                if (snark.isStarting())
+                    return 15;
+                if (snark.isAllocating())
+                    return 20;
                 if (remaining < 0)
+                    return 45; // magnet
+                if (remaining == 0) { //seeding
+                    if (activePeers > 0)
+                        return 65; // active
+                    else if (peers > 0) // inactive with swarm members
+                        return 98;
+                    else
+                        return 100;
+                }
+                if (snark.isChecking())
+                    return 95;
+                if (snark.getNeededLength() <= 0)https://news.google.com/topstories?hl=en-US&gl=US&ceid=US:en
+                    return 90;
+                if (peers <= 0) // no peers in swarm
+                    return 35;
+                if (activePeers <= 0) // no active peers
                     return 10;
-                if (remaining > 0)
+                if (downBps <= 0) // stalled
+                    return 7;
+                else
                     return 5;
-                return 0;
-            }
-            if (snark.isStarting())
-                return 15;
-            if (snark.isAllocating())
-                return 20;
-            if (remaining < 0)
-                return 15; // magnet
-            if (remaining == 0)
-                return 100;
-            if (snark.isChecking())
-                return 95;
-            if (snark.getNeededLength() <= 0)
-                return 90;
-            if (snark.getPeerCount() <= 0)
-                return 40;
-            if (snark.getDownloadRate() <= 0)
-                return 50;
-            return 60;
-        }
+             }
+         }
     }
 
     private static class PeersComparator extends Sort {
@@ -256,20 +280,49 @@ class Sorters {
         public ETAComparator(boolean rev, String lang) { super(rev, lang); }
 
         public int compareIt(Snark l, Snark r) {
+            // TODO For completed torrents, sort by date of completion
             return compLong(eta(l), eta(r));
         }
 
         private static long eta(Snark snark) {
-            long needed = snark.getNeededLength(); 
-            if (needed <= 0)
-                return 0;
-            long total = snark.getTotalLength();
-            if (needed > total)
-                needed = total;
-            long downBps = snark.getDownloadRate();
-            if (downBps > 0)
-                return needed / downBps;
-            return Long.MAX_VALUE;
+            long needed = snark.getNeededLength();
+            long remaining = snark.getRemainingLength();
+            long upBps = snark.getUploadRate();
+            int activePeers = snark.getPeerCount();
+            int peers = snark.getTrackerSeenPeers();
+            if (snark.isStopped()) {
+                if (remaining == 0)
+                    return Long.MAX_VALUE - 9;
+                if (remaining < 0) // magnet
+                    return Long.MAX_VALUE - 10;
+                else
+                    return Long.MAX_VALUE - 11;
+            } else {
+                if (remaining < 0) // magnet
+                    return Long.MAX_VALUE - 12;
+                else if (remaining == 0) {
+                    if (upBps > 0)
+                        return Long.MAX_VALUE - 8;
+                    if (activePeers > 0)
+                        return Long.MAX_VALUE - 7;
+                    if (peers > 0)
+                        return Long.MAX_VALUE - 6;
+                }
+                if (needed > 0 && snark.getDownloadRate() <= 0) {
+                    if (activePeers <= 0)
+                        return Long.MAX_VALUE - 16;
+                    else
+                        return Long.MAX_VALUE - 17;
+                }
+                long total = snark.getTotalLength();
+                if (needed > total)
+                   needed = total;
+                long downBps = snark.getDownloadRate();
+                if (downBps > 0)
+                    return needed / downBps;
+                else
+                    return Long.MAX_VALUE - 19;
+            }
         }
     }

Updated: 27 May 2020

Subtickets

Change History (11)

comment:1 Changed 6 months ago by zzz

-            if (rv != 0)
+//            if (rv != 0)
+            if (rv != 50)

That's not how to write a comparator. If the previous comparison is equal to zero, then you fall through to the next comparison as a tie breaker. You can't do it like that.

Also, please provide a diff that does not comment things out, it just makes a mess. Just change the line you need. The diff format makes it very clear without commented lines.

-            return compLong(eta(l), eta(r));
+            return compLong(eta(r), eta(l));

this should instead be done by flipping the -4 and 4 in the servlet, like you did for -2 and 2. Be consistent.

+            if (snark.isStopped() && remaining == 0)
+                return Long.MAX_VALUE - 2;
+            if (snark.isStopped() && remaining < 0) // magnet
+                return Long.MAX_VALUE - 7;
+            if (snark.isStopped())
+                return Long.MAX_VALUE - 3;

This should be cleaned up to only call isStopped() once, then do the other checks.

comment:2 Changed 6 months ago by Reportage

diff --git a/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java b/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java
index fcae72def..ade42f77a 100644
--- a/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java
+++ b/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java
@@ -493,7 +493,7 @@ public class I2PSnarkServlet extends BasicServlet {
         String currentSort = req.getParameter("sort");
         boolean showSort = total > 1;
         out.write("<tr><th class=\"snarkGraphicStatus\">");
-        String sort = ("-2".equals(currentSort)) ? "2" : "-2";
+        String sort = ("2".equals(currentSort)) ? "-2" : "2";
         if (showSort) {
             out.write("<a href=\"" + _contextPath + '/' + getQueryString(req, null, null, sort));
             out.write("\">");
@@ -563,7 +563,7 @@ public class I2PSnarkServlet extends BasicServlet {
         out.write("</th>\n<th class=\"snarkTorrentETA\" align=\"right\">");
         if (_manager.util().connected() && !snarks.isEmpty()) {
             if (showSort) {
-                sort = ("-4".equals(currentSort)) ? "4" : "-4";
+                sort = ("4".equals(currentSort)) ? "-4" : "4";
                 out.write("<a href=\"" + _contextPath + '/' + getQueryString(req, null, null, sort));
                 out.write("\">");
             }
diff --git a/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java b/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
index 56c3115e7..7d2a6cbdb 100644
--- a/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
+++ b/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
@@ -200,25 +200,28 @@ class Sorters {
             int rv = getStatus(l) - getStatus(r);
             if (rv != 0)
                 return rv;
-            // use reverse remaining as first tie break
-            return compLong(r.getNeededLength(), l.getNeededLength());
+            // use reverse peercount as first tie break
+            return compLong(l.getPeerCount(), r.getPeerCount());
         }
 
         private static int getStatus(Snark snark) {
-            long remaining = snark.getRemainingLength(); 
+            long remaining = snark.getRemainingLength();
             if (snark.isStopped()) {
                 if (remaining < 0)
-                    return 10;
+                    return 50;
                 if (remaining > 0)
-                    return 5;
-                return 0;
+                    return 46;
+                else
+                    return 60;
             }
             if (snark.isStarting())
                 return 15;
             if (snark.isAllocating())
                 return 20;
             if (remaining < 0)
-                return 15; // magnet
+                return 45; // magnet
+            if (remaining == 0 && snark.getPeerCount() > 0)
+                return 99; // seeding torrents with peers
             if (remaining == 0)
                 return 100;
             if (snark.isChecking())
@@ -228,8 +231,9 @@ class Sorters {
             if (snark.getPeerCount() <= 0)
                 return 40;
             if (snark.getDownloadRate() <= 0)
-                return 50;
-            return 60;
+                return 35;
+            else
+                return 5;
         }
     }
 
@@ -260,16 +264,33 @@ class Sorters {
         }
 
         private static long eta(Snark snark) {
-            long needed = snark.getNeededLength(); 
-            if (needed <= 0)
-                return 0;
-            long total = snark.getTotalLength();
-            if (needed > total)
-                needed = total;
-            long downBps = snark.getDownloadRate();
-            if (downBps > 0)
-                return needed / downBps;
-            return Long.MAX_VALUE;
+            long needed = snark.getNeededLength();
+            long remaining = snark.getRemainingLength();
+            if (snark.isStopped()) {
+                if (remaining == 0)
+                    return Long.MAX_VALUE - 2;
+                if (remaining < 0) // magnet
+                    return Long.MAX_VALUE - 7;
+                else
+                    return Long.MAX_VALUE - 3;
+            } else {
+                if (remaining < 0) // magnet
+                    return Long.MAX_VALUE - 8;
+                if (needed > 0 && snark.getDownloadRate() <= 0) {
+                    if (snark.getPeerCount() <= 0)
+                        return Long.MAX_VALUE - 9;
+                    else
+                        return Long.MAX_VALUE - 10;
+                }
+                long total = snark.getTotalLength();
+                if (needed > total)
+                   needed = total;
+                long downBps = snark.getDownloadRate();
+                if (downBps > 0)
+                    return needed / downBps;
+                else
+                    return Long.MAX_VALUE - 1;
+            }
         }
     }

comment:3 Changed 6 months ago by Reportage

Description: modified (diff)

comment:4 Changed 6 months ago by Reportage

Description: modified (diff)

comment:5 Changed 6 months ago by Reportage

Description: modified (diff)

comment:6 Changed 6 months ago by Reportage

Resolved outstanding sorting issues. Now status sort will order inactive seeding torrents by number of peers in swarm, and other mods.

diff --git a/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java b/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java
index fcae72def..ade42f77a 100644
--- a/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java
+++ b/apps/i2psnark/java/src/org/klomp/snark/web/I2PSnarkServlet.java
@@ -493,7 +493,7 @@ public class I2PSnarkServlet extends BasicServlet {
         String currentSort = req.getParameter("sort");
         boolean showSort = total > 1;
         out.write("<tr><th class=\"snarkGraphicStatus\">");
-        String sort = ("-2".equals(currentSort)) ? "2" : "-2";
+        String sort = ("2".equals(currentSort)) ? "-2" : "2";
         if (showSort) {
             out.write("<a href=\"" + _contextPath + '/' + getQueryString(req, null, null, sort));
             out.write("\">");
@@ -563,7 +563,7 @@ public class I2PSnarkServlet extends BasicServlet {
         out.write("</th>\n<th class=\"snarkTorrentETA\" align=\"right\">");
         if (_manager.util().connected() && !snarks.isEmpty()) {
             if (showSort) {
-                sort = ("-4".equals(currentSort)) ? "4" : "-4";
+                sort = ("4".equals(currentSort)) ? "-4" : "4";
                 out.write("<a href=\"" + _contextPath + '/' + getQueryString(req, null, null, sort));
                 out.write("\">");
             }
diff --git a/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java b/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
index 56c3115e7..bfdb7b228 100644
--- a/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
+++ b/apps/i2psnark/java/src/org/klomp/snark/web/Sorters.java
@@ -200,27 +200,36 @@ class Sorters {
             int rv = getStatus(l) - getStatus(r);
             if (rv != 0)
                 return rv;
-            // use reverse remaining as first tie break
-            return compLong(r.getNeededLength(), l.getNeededLength());
+            else if (getStatus(l) == 100 && getStatus(r) == 100)
+                // first tie break sorts by peer count
+                return compLong(r.getTrackerSeenPeers(), l.getTrackerSeenPeers());
+            else
+                // first tie break sorts by peer count
+                return compLong(r.getPeerCount(), l.getPeerCount());
         }
 
         private static int getStatus(Snark snark) {
             long remaining = snark.getRemainingLength();
             if (snark.isStopped()) {
                 if (remaining < 0)
-                    return 10;
+                    return 50;
                 if (remaining > 0)
-                    return 5;
-                return 0;
+                    return 46;
+                else
+                    return 60;
             }
             if (snark.isStarting())
                 return 15;
             if (snark.isAllocating())
                 return 20;
             if (remaining < 0)
-                return 15; // magnet
-            if (remaining == 0)
+                return 45; // magnet
+            if (remaining == 0) {
+                if (snark.getPeerCount() > 0)
+                    return 99; // seeding torrents with peers
+                else
                     return 100;
+            }
             if (snark.isChecking())
                 return 95;
             if (snark.getNeededLength() <= 0)
@@ -228,8 +237,9 @@ class Sorters {
             if (snark.getPeerCount() <= 0)
                 return 40;
             if (snark.getDownloadRate() <= 0)
-                return 50;
-            return 60;
+                return 35;
+            else
+                return 5;
         }
     }
 
@@ -261,15 +271,43 @@ class Sorters {
 
         private static long eta(Snark snark) {
             long needed = snark.getNeededLength();
-            if (needed <= 0)
-                return 0;
+            long remaining = snark.getRemainingLength();
+            long upBps = snark.getUploadRate();
+            int activePeers = snark.getPeerCount();
+            int peers = snark.getTrackerSeenPeers();
+            if (snark.isStopped()) {
+                if (remaining == 0)
+                    return Long.MAX_VALUE - 9;
+                if (remaining < 0) // magnet
+                    return Long.MAX_VALUE - 10;
+                else
+                    return Long.MAX_VALUE - 11;
+            } else {
+                if (remaining < 0) // magnet
+                    return Long.MAX_VALUE - 12;
+                else if (remaining == 0) {
+                    if (upBps > 0)
+                        return Long.MAX_VALUE - 8;
+                    if (activePeers > 0)
+                        return Long.MAX_VALUE - 7;
+                    if (peers > 0)
+                        return Long.MAX_VALUE - 6;
+                }
+                if (needed > 0 && snark.getDownloadRate() <= 0) {
+                    if (activePeers <= 0)
+                        return Long.MAX_VALUE - 16;
+                    else
+                        return Long.MAX_VALUE - 17;
+                }
                 long total = snark.getTotalLength();
                 if (needed > total)
                    needed = total;
                 long downBps = snark.getDownloadRate();
                 if (downBps > 0)
                     return needed / downBps;
-            return Long.MAX_VALUE;
+                else
+                    return Long.MAX_VALUE - 19;
+            }
         }
     }

comment:7 Changed 5 months ago by zzz

I've been testing this for a while. The goal, I think, is for the first click on a sort column to bring the most 'interesting' ones to the top. That's especially the goal for the mitsubishi, and that's what I tried to do in the current code. My definition of 'interesting' is that anything that's running or incomplete is more interesting that ones that are stopped and completed. People are most likely to want to check the status of their running torrents.

The issue I have with the patch is that it puts running leeches on one side of the sort and running seeds on the other, with stopped completed torrents in the middle. So I can't see all my running torrents on either end of the sort.

If we agree that stopped completed torrents are the least interesting, I don't have a strong opinion on the sort order within running torrents, but I think our goal is to put the ones people most want to see the status of at the top. That's why magnets always go to the top in my sort, because that's hopefully a transient state.

comment:8 Changed 5 months ago by Reportage

Description: modified (diff)

comment:9 Changed 5 months ago by zzz

Tested the latest version in OP, it still has leeches at one end, seeds on the other, and stopped completed torrents in the middle, so it still fails the goal I described in comment 7.

comment:10 Changed 5 months ago by Reportage

Status sort (descending order) is as follows:

  • Downloading (sub-sort by seed count)
    • Active
    • Stalled
    • No Peers
    • Magnet/torrent links
    • Stopped (incomplete)
  • Completed (stopped)
  • Seeding
    • Actively seeding (sub-sort by leech count)
    • Inactive seeding (sub-sort by swarm size)

In terms of importance, I'm ranking stopped, completed torrents before seeding torrents because they're more noteworthy and may require attention; torrents that are seeding are arguably less important, status-wise, because they're less likely to require attention.

If you want to sort purely by the number of connected peers, that could be an alternative sort algorithm that's triggered on the 3rd/4th click of the status icon in similar vein to the most of the other sorters, in which case case sorting by a) number of connected peers and b) swarm size might give you what you want.

comment:11 Changed 4 months ago by zzz

Changed the default order for ETA sort (4) as you recommended, in 3d5e3e6a0c3819d17e5e1eef31dede5c023e02e7 to be 0.9.46-8

As to the status sort (2), I think we're at a point where we're going to agree to disagree. Thanks for the explanation above, but your design in comment 10 is not compatible with my goals in comment 7.

One other case I saw recently was where a running torrent with no peers was on one side of the sort, while a running torrent with 1 peer was on the other side. That's consistent with your explanation in comment 10 and inconsistent with my goals in comment 7.

I propose closing this ticket as (mostly) wontfix, but if there's any other sort changes you think I should take let me know.

Note: See TracTickets for help on using tickets.