Merge pull request #8751 from bjorng/bjorn/erts/fix-delay-gc/OTP-12909

Fix performance regression caused by a bug fix

Author: bjorng

erts/emulator/beam/beam_bp.c

@@ -927,8 +927,6 @@ static void restore_cp_after_trace(Process *c_p, const Eterm cp_save[2]) {
 }
 
 static ERTS_INLINE Uint get_allocated_words(Process *c_p, Sint allocated) {
-    if (c_p->abandoned_heap)
-        return allocated + c_p->htop - c_p->heap + c_p->mbuf_sz;
     return allocated + c_p->htop - c_p->high_water + c_p->mbuf_sz;
 }
 

erts/emulator/beam/erl_bif_trace.c

@@ -2908,12 +2908,10 @@ new_seq_trace_token(Process* p, int ensure_new_heap)
 				    make_small(p->seq_trace_lastcnt));
     }
     else if (ensure_new_heap) {
-        Eterm *mature = p->abandoned_heap ? p->abandoned_heap : p->heap;
-        Uint mature_size = p->high_water - mature;
         Eterm* tpl = tuple_val(SEQ_TRACE_TOKEN(p));
         ASSERT(arityval(tpl[0]) == 5);
-        if (ErtsInBetween(tpl, OLD_HEAP(p), OLD_HEND(p)) ||
-            ErtsInArea(tpl, mature, mature_size*sizeof(Eterm))) {
+
+        if (!ErtsInBetween(tpl, p->high_water, p->hend)) {
             hp = HAlloc(p, 6);
             sys_memcpy(hp, tpl, 6*sizeof(Eterm));
             SEQ_TRACE_TOKEN(p) = make_tuple(hp);

erts/emulator/beam/erl_gc.c

@@ -90,7 +90,7 @@ do {									\
     ASSERT((p)->abandoned_heap || (P)->heap_sz == (P)->hend - (P)->heap); \
     ASSERT((P)->heap <= (P)->htop && (P)->htop <= (P)->hend);		\
     ASSERT((P)->heap <= (P)->stop && (P)->stop <= (P)->hend);		\
-    ASSERT((p)->abandoned_heap || ((P)->heap <= (P)->high_water && (P)->high_water <= (P)->hend)); \
+    ASSERT(((P)->heap <= (P)->high_water && (P)->high_water <= (P)->hend)); \
     OverRunCheck((P));							\
 } while (0)
 #else
@@ -525,12 +525,28 @@ delay_garbage_collection(Process *p, int need, int fcalls)
     ssz = orig_hend - orig_stop;
     hsz = ssz + need + ERTS_DELAY_GC_EXTRA_FREE + S_RESERVED;
 
-    hfrag = new_message_buffer(hsz);
+    /* Allocate one extra word at the end to save the high water mark. */
+    hfrag = new_message_buffer(hsz + 1);
 
     copy_erlang_stack(p, &hfrag->mem[0], hsz);
 
     p->heap = p->htop = &hfrag->mem[0];
-    p->hend = hend = &hfrag->mem[hsz];
+    hend = &hfrag->mem[hsz];
+
+    /* Save the original high water mark at the end of the current
+     * heap to make it possible to do a minor GC later. */
+    if (p->abandoned_heap) {
+        *hend = (Eterm) (p->hend[0]);
+    } else {
+        *hend = (Eterm) p->high_water;
+    }
+
+    p->hend = hend;
+
+    /* Keep the high water mark pointing into the current heap to ensure
+     * that the test for the safe range in the update_record_in_place (JIT)
+     * stays honest. */
+    p->high_water = p->heap;
 
     if (p->abandoned_heap) {
 	/*
@@ -543,7 +559,7 @@ delay_garbage_collection(Process *p, int need, int fcalls)
         Uint used = orig_htop - orig_heap;
         hfrag->used_size = used;
         p->mbuf_sz += used;
-        ASSERT(hfrag->used_size <= hfrag->alloc_size);
+        ASSERT(hfrag->used_size <= hfrag->alloc_size-1);
         ASSERT(!hfrag->off_heap.first && !hfrag->off_heap.overhead);
         hfrag->next = p->mbuf;
         p->mbuf = hfrag;
@@ -559,11 +575,6 @@ delay_garbage_collection(Process *p, int need, int fcalls)
 	}
 	p->abandoned_heap = orig_heap;
         erts_adjust_memory_break(p, orig_htop - p->high_water);
-
-        /* Point at the end of the address range to ensure that
-         * test for the safe range in the new heap in the
-         * update_record_in_place instruction fails. */
-        p->high_water = (Eterm *) (Uint) -1;
     }
 
 #ifdef CHECK_FOR_HOLES
@@ -637,21 +648,39 @@ young_gen_usage(Process *p, Uint *ext_msg_usage)
     return hsz;
 }
 
-#define ERTS_GET_ORIG_HEAP(Proc, Heap, HTop)			\
-    do {							\
-	Eterm *aheap__ = (Proc)->abandoned_heap;		\
-	if (!aheap__) {						\
-	    (Heap) = (Proc)->heap;				\
-	    (HTop) = (Proc)->htop;				\
-	}							\
-	else {							\
-	    (Heap) = aheap__;					\
-	    if ((Proc)->flags & F_ABANDONED_HEAP_USE)		\
-		(HTop) = aheap__ + aheap__[(Proc)->heap_sz-1];	\
-	    else						\
-		(HTop) = aheap__ + (Proc)->heap_sz;		\
-	}							\
-    } while (0)
+static Eterm*
+get_orig_heap(Process *p, Eterm **p_htop, Eterm **p_high_water) {
+    Eterm *aheap = p->abandoned_heap;
+    Eterm *htop;
+
+    /* See delay_garbage_collection(). */
+
+    ASSERT(aheap != NULL);
+
+    if (p->flags & F_ABANDONED_HEAP_USE) {
+        htop = aheap + aheap[p->heap_sz-1];
+    } else {
+        htop = aheap + p->heap_sz;
+    }
+
+    *p_htop = htop;
+
+    if (p_high_water) {
+        Eterm *high_water;
+
+        high_water = (Eterm *)(p->hend[0]);
+
+        ASSERT(aheap <= high_water);
+        ASSERT(high_water <= htop);
+
+        /* The high water pointer must be aligned to a word boundary. */
+        ASSERT(((UWord) high_water) % sizeof(UWord) == 0);
+
+        *p_high_water = high_water;
+    }
+
+    return aheap;
+}
 
 static ERTS_INLINE void
 check_for_possibly_long_gc(Process *p, Uint ygen_usage)
@@ -1383,12 +1412,32 @@ minor_collection(Process* p, ErlHeapFragment *live_hf_end,
 		 Uint ygen_usage, Uint *recl)
 {
     Eterm *mature = p->abandoned_heap ? p->abandoned_heap : p->heap;
-    Uint mature_size = p->high_water - mature;
+    Eterm *high_water;
+    Uint mature_size;
     Uint size_before = ygen_usage;
 #ifdef DEBUG
     Uint debug_tmp = 0;
 #endif
 
+    if (p->abandoned_heap) {
+        /* See delay_garbage_collection(). */
+        high_water = (Eterm *)(p->hend[0]);
+    } else {
+        high_water = p->high_water;
+    }
+
+#ifdef DEBUG
+    if (p->abandoned_heap) {
+        ASSERT(p->abandoned_heap <= high_water);
+        ASSERT(high_water - p->abandoned_heap <= size_before);
+
+        /* The high water pointer must be aligned to a word boundary. */
+        ASSERT(((UWord) high_water) % sizeof(UWord) == 0);
+    }
+#endif
+
+    mature_size = high_water - mature;
+
     need += S_RESERVED;
 
     /*
@@ -3659,9 +3708,11 @@ erts_process_gc_info(Process *p, Uint *sizep, Eterm **hpp,
     ERTS_CT_ASSERT(sizeof(values)/sizeof(*values) == ERTS_PROCESS_GC_INFO_MAX_TERMS);
 
     if (p->abandoned_heap) {
-        Eterm *htop, *heap;
-        ERTS_GET_ORIG_HEAP(p, heap, htop);
-        values[3] = HIGH_WATER(p) - heap;
+        Eterm *htop, *heap, *high_water;
+
+        heap = get_orig_heap(p, &htop, &high_water);
+
+        values[3] = high_water - heap;
         values[6] = htop - heap;
     }
 
@@ -3906,7 +3957,7 @@ erts_dbg_within_proc(Eterm *ptr, Process *p, Eterm *real_htop)
     Eterm *htop, *heap;
 
     if (p->abandoned_heap) {
-	ERTS_GET_ORIG_HEAP(p, heap, htop);
+        heap = get_orig_heap(p, &htop, NULL);
 	if (heap <= ptr && ptr < htop)
 	    return 1;
     }
@@ -4017,7 +4068,7 @@ erts_dbg_check_heap_terms(int (*check_eterm)(Eterm),
     Eterm *htop, *heap;
 
     if (p->abandoned_heap) {
-	ERTS_GET_ORIG_HEAP(p, heap, htop);
+        heap = get_orig_heap(p, &htop, NULL);
 	if (!check_all_heap_terms_in_range(check_eterm,
                                            heap, htop))
 	    return 0;

erts/emulator/beam/erl_process.h

@@ -1057,7 +1057,15 @@ struct process {
 
     Eterm* heap;                /* Heap start */
     Eterm* hend;                /* Heap end */
+
+    /* If abandoned_heap is not a NULL pointer, it points to the heap
+     * that was active when delay_garbage_collection() in erl_gc.c was
+     * called. The high water mark that was active at that time is
+     * saved in p->hend[0].
+     */
+
     Eterm* abandoned_heap;
+
     Uint heap_sz;               /* Size of heap in words */
     Uint min_heap_size;         /* Minimum size of heap (in words). */
     Uint min_vheap_size;        /* Minimum size of virtual heap (in words). */