Re: Slowdown around 5.0.5-dev-0

[Bart Schaefer <schaefer@xxxxxxxxxxxxxxxx>]

On Oct 23,  8:32am, Sebastian Gniazdowski wrote:
} Subject: Re: Slowdown around 5.0.5-dev-0
}
} 
} [ text/plain
}   Encoded with "quoted-printable" ] :
} 
} I ran the performance tests again with script formatting them
} differently - they are aggregated on test function, not on zsh
} version. This allows easier interpretation. A new conclusion is that
} zhalloc patch doesn't help for string_test. So the first patch (36834)
} speeds up string operations and also introduces increased memory
} usage.

Thanks for the patch.  I think you've removed too much; the part about
finding the arena with the most space rather than first arena with any
space should have stayed, as should the changes to zhalloc() that were
included in 36834.

Try the below patch against 8e9a68ad1 ?


On Oct 23,  5:57pm, Sebastian Gniazdowski wrote:
} Subject: Re: Slowdown around 5.0.5-dev-0
}
} On 23 October 2015 at 17:40, Sebastian Gniazdowski
} <sgniazdowski@xxxxxxxxx> wrote:
} > I attach the patch that withdraws 9f8e3e8 and 643aca9 (which are 36834)
} 
} One note is that 643aca9 isn't fully from 36834 because:
}            if (fheap == h)
}                fheap = NULL;
} 
} Is moved to top of "if (h->next) {". Was this intended?

Yes it was, but irrelevant since that whole block is being backed out.


diff --git a/Src/mem.c b/Src/mem.c
index 68bd767..15d8fd2 100644
--- a/Src/mem.c
+++ b/Src/mem.c
@@ -294,7 +294,7 @@ pushheap(void)
 #endif
 
     for (h = heaps; h; h = h->next) {
-	DPUTS(!h->used && h->next, "BUG: empty heap");
+	DPUTS(!h->used, "BUG: empty heap");
 	hs = (Heapstack) zalloc(sizeof(*hs));
 	hs->next = h->sp;
 	h->sp = hs;
@@ -339,10 +339,12 @@ freeheap(void)
      * of stuff already on the heap, this is an enormous amount of work,
      * and performance goes to hell.
      *
-     * Therefore, we defer freeing the most recently allocated arena until
-     * we reach popheap().  This may fail to reclaim some space in earlier
-     * arenas.
-     *
+     * However, if the arena to which fheap points is unused, we want to
+     * free it, so we have no choice but to do the sweep for a new fheap.
+     */
+    if (fheap && !fheap->sp)
+	fheap = NULL;	/* We used to do this unconditionally */
+    /*
      * In other cases, either fheap is already correct, or it has never
      * been set and this loop will do it, or it'll be reset from scratch
      * on the next popheap().  So all that's needed here is to pick up
@@ -386,26 +388,6 @@ freeheap(void)
 	    VALGRIND_MEMPOOL_TRIM((char *)h, (char *)arena(h), h->used);
 #endif
 	} else {
-	    if (fheap == h)
-		fheap = NULL;
-	    if (h->next) {
-		/* We want to cut this out of the arena list if we can */
-		if (h == heaps)
-		    hl = heaps = h->next;
-		else if (hl && hl->next == h)
-		    hl->next = h->next;
-		else {
-		    DPUTS(hl, "hl->next != h when freeing");
-		    hl = h;
-		    continue;
-		}
-		h->next = NULL;
-	    } else {
-		/* Leave an empty arena at the end until popped */
-		h->used = 0;
-		fheap = hl = h;
-		break;
-	    }
 #ifdef USE_MMAP
 	    munmap((void *) h, h->size);
 #else
@@ -473,19 +455,6 @@ popheap(void)
 
 	    hl = h;
 	} else {
-	    if (h->next) {
-		/* We want to cut this out of the arena list if we can */
-		if (h == heaps)
-		    hl = heaps = h->next;
-		else if (hl && hl->next == h)
-		    hl->next = h->next;
-		else {
-		    DPUTS(hl, "hl->next != h when popping");
-		    hl = h;
-		    continue;
-		}
-		h->next = NULL;
-	    }
 #ifdef USE_MMAP
 	    munmap((void *) h, h->size);
 #else