Fix prof_realloc() regression.

Mostly revert the prof_realloc() changes in
498856f44a (Move slabs out of chunks.) so
that prof_free_sampled_object() is called when appropriate.  Leave the
prof_tctx_[re]set() optimization in place, but add an assertion to
verify that all eight cases are correctly handled.  Add a comment to
make clear the code ordering, so that the regression originally fixed by
ea8d97b897 (Fix
prof_{malloc,free}_sample_object() call order in prof_realloc().) is not
repeated.

This resolves #499.

Makefile.in

@@ -179,6 +179,7 @@ TESTS_UNIT := \
 	$(srcroot)test/unit/prof_gdump.c \
 	$(srcroot)test/unit/prof_idump.c \
 	$(srcroot)test/unit/prof_reset.c \
+	$(srcroot)test/unit/prof_tctx.c \
 	$(srcroot)test/unit/prof_thread_name.c \
 	$(srcroot)test/unit/ql.c \
 	$(srcroot)test/unit/qr.c \

include/jemalloc/internal/private_symbols.txt

@@ -369,6 +369,7 @@ prof_boot0
 prof_boot1
 prof_boot2
 prof_bt_count
+prof_cnt_all
 prof_dump_header
 prof_dump_open
 prof_free

include/jemalloc/internal/prof_externs.h

@@ -48,11 +48,12 @@ prof_tctx_t	*prof_lookup(tsd_t *tsd, prof_bt_t *bt);
 #ifdef JEMALLOC_JET
 size_t	prof_tdata_count(void);
 size_t	prof_bt_count(void);
-const prof_cnt_t *prof_cnt_all(void);
 typedef int (prof_dump_open_t)(bool, const char *);
 extern prof_dump_open_t *prof_dump_open;
 typedef bool (prof_dump_header_t)(tsdn_t *, bool, const prof_cnt_t *);
 extern prof_dump_header_t *prof_dump_header;
+void	prof_cnt_all(uint64_t *curobjs, uint64_t *curbytes,
+    uint64_t *accumobjs, uint64_t *accumbytes);
 #endif
 void	prof_idump(tsdn_t *tsdn);
 bool	prof_mdump(tsd_t *tsd, const char *filename);

include/jemalloc/internal/prof_inlines.h

@@ -194,30 +194,39 @@ prof_realloc(tsd_t *tsd, extent_t *extent, const void *ptr, size_t usize,
 		}
 	}
 
-	/*
-	 * The following code must differentiate among eight possible cases,
-	 * based on three boolean conditions.
-	 */
 	sampled = ((uintptr_t)tctx > (uintptr_t)1U);
 	old_sampled = ((uintptr_t)old_tctx > (uintptr_t)1U);
 	moved = (ptr != old_ptr);
 
-	/*
-	 * The following block must only execute if this is a non-moving
-	 * reallocation, because for moving reallocation the old allocation will
-	 * be deallocated via a separate call.
-	 */
-	if (unlikely(old_sampled) && !moved)
-		prof_free_sampled_object(tsd, old_usize, old_tctx);
-
 	if (unlikely(sampled)) {
 		prof_malloc_sample_object(tsd_tsdn(tsd), extent, ptr, usize,
 		    tctx);
 	} else if (moved) {
 		prof_tctx_set(tsd_tsdn(tsd), extent, ptr, usize,
 		    (prof_tctx_t *)(uintptr_t)1U);
-	} else if (unlikely(old_sampled))
+	} else if (unlikely(old_sampled)) {
+		/*
+		 * prof_tctx_set() would work for the !moved case as well, but
+		 * prof_tctx_reset() is slightly cheaper, and the proper thing
+		 * to do here in the presence of explicit knowledge re: moved
+		 * state.
+		 */
 		prof_tctx_reset(tsd_tsdn(tsd), extent, ptr, tctx);
+	} else {
+		assert((uintptr_t)prof_tctx_get(tsd_tsdn(tsd), extent, ptr) ==
+		    (uintptr_t)1U);
+	}
+
+	/*
+	 * The prof_free_sampled_object() call must come after the
+	 * prof_malloc_sample_object() call, because tctx and old_tctx may be
+	 * the same, in which case reversing the call order could cause the tctx
+	 * to be prematurely destroyed as a side effect of momentarily zeroed
+	 * counters.
+	 */
+	if (unlikely(old_sampled)) {
+		prof_free_sampled_object(tsd, old_usize, old_tctx);
+	}
 }
 
 JEMALLOC_ALWAYS_INLINE void

src/prof.c

@@ -1530,95 +1530,178 @@ label_return:
 	return (ret);
 }
 
-static bool
-prof_dump(tsd_t *tsd, bool propagate_err, const char *filename, bool leakcheck)
+static void
+prof_dump_prep(tsd_t *tsd, prof_tdata_t *tdata,
+    struct prof_tdata_merge_iter_arg_s *prof_tdata_merge_iter_arg,
+    struct prof_gctx_merge_iter_arg_s *prof_gctx_merge_iter_arg,
+    prof_gctx_tree_t *gctxs)
 {
-	prof_tdata_t *tdata;
-	struct prof_tdata_merge_iter_arg_s prof_tdata_merge_iter_arg;
 	size_t tabind;
 	union {
 		prof_gctx_t	*p;
 		void		*v;
 	} gctx;
-	struct prof_gctx_merge_iter_arg_s prof_gctx_merge_iter_arg;
-	struct prof_gctx_dump_iter_arg_s prof_gctx_dump_iter_arg;
-	prof_gctx_tree_t gctxs;
 
-	cassert(config_prof);
-
-	tdata = prof_tdata_get(tsd, true);
-	if (tdata == NULL)
-		return (true);
-
-	malloc_mutex_lock(tsd_tsdn(tsd), &prof_dump_mtx);
 	prof_enter(tsd, tdata);
 
 	/*
 	 * Put gctx's in limbo and clear their counters in preparation for
 	 * summing.
 	 */
-	gctx_tree_new(&gctxs);
-	for (tabind = 0; !ckh_iter(&bt2gctx, &tabind, NULL, &gctx.v);)
-		prof_dump_gctx_prep(tsd_tsdn(tsd), gctx.p, &gctxs);
+	gctx_tree_new(gctxs);
+	for (tabind = 0; !ckh_iter(&bt2gctx, &tabind, NULL, &gctx.v);) {
+		prof_dump_gctx_prep(tsd_tsdn(tsd), gctx.p, gctxs);
+	}
 
 	/*
 	 * Iterate over tdatas, and for the non-expired ones snapshot their tctx
 	 * stats and merge them into the associated gctx's.
 	 */
-	prof_tdata_merge_iter_arg.tsdn = tsd_tsdn(tsd);
-	memset(&prof_tdata_merge_iter_arg.cnt_all, 0, sizeof(prof_cnt_t));
+	prof_tdata_merge_iter_arg->tsdn = tsd_tsdn(tsd);
+	memset(&prof_tdata_merge_iter_arg->cnt_all, 0, sizeof(prof_cnt_t));
 	malloc_mutex_lock(tsd_tsdn(tsd), &tdatas_mtx);
 	tdata_tree_iter(&tdatas, NULL, prof_tdata_merge_iter,
-	    (void *)&prof_tdata_merge_iter_arg);
+	    (void *)prof_tdata_merge_iter_arg);
 	malloc_mutex_unlock(tsd_tsdn(tsd), &tdatas_mtx);
 
 	/* Merge tctx stats into gctx's. */
-	prof_gctx_merge_iter_arg.tsdn = tsd_tsdn(tsd);
-	prof_gctx_merge_iter_arg.leak_ngctx = 0;
-	gctx_tree_iter(&gctxs, NULL, prof_gctx_merge_iter,
-	    (void *)&prof_gctx_merge_iter_arg);
+	prof_gctx_merge_iter_arg->tsdn = tsd_tsdn(tsd);
+	prof_gctx_merge_iter_arg->leak_ngctx = 0;
+	gctx_tree_iter(gctxs, NULL, prof_gctx_merge_iter,
+	    (void *)prof_gctx_merge_iter_arg);
 
 	prof_leave(tsd, tdata);
+}
 
+static bool
+prof_dump_file(tsd_t *tsd, bool propagate_err, const char *filename,
+    bool leakcheck, prof_tdata_t *tdata,
+    struct prof_tdata_merge_iter_arg_s *prof_tdata_merge_iter_arg,
+    struct prof_gctx_merge_iter_arg_s *prof_gctx_merge_iter_arg,
+    struct prof_gctx_dump_iter_arg_s *prof_gctx_dump_iter_arg,
+    prof_gctx_tree_t *gctxs)
+{
 	/* Create dump file. */
-	if ((prof_dump_fd = prof_dump_open(propagate_err, filename)) == -1)
-		goto label_open_close_error;
+	if ((prof_dump_fd = prof_dump_open(propagate_err, filename)) == -1) {
+		return true;
+	}
 
 	/* Dump profile header. */
 	if (prof_dump_header(tsd_tsdn(tsd), propagate_err,
-	    &prof_tdata_merge_iter_arg.cnt_all))
+	    &prof_tdata_merge_iter_arg->cnt_all)) {
 		goto label_write_error;
+	}
 
 	/* Dump per gctx profile stats. */
-	prof_gctx_dump_iter_arg.tsdn = tsd_tsdn(tsd);
-	prof_gctx_dump_iter_arg.propagate_err = propagate_err;
-	if (gctx_tree_iter(&gctxs, NULL, prof_gctx_dump_iter,
-	    (void *)&prof_gctx_dump_iter_arg) != NULL)
+	prof_gctx_dump_iter_arg->tsdn = tsd_tsdn(tsd);
+	prof_gctx_dump_iter_arg->propagate_err = propagate_err;
+	if (gctx_tree_iter(gctxs, NULL, prof_gctx_dump_iter,
+	    (void *)prof_gctx_dump_iter_arg) != NULL) {
 		goto label_write_error;
+	}
 
 	/* Dump /proc/<pid>/maps if possible. */
-	if (prof_dump_maps(propagate_err))
+	if (prof_dump_maps(propagate_err)) {
 		goto label_write_error;
+	}
 
-	if (prof_dump_close(propagate_err))
-		goto label_open_close_error;
+	if (prof_dump_close(propagate_err)) {
+		return true;
+	}
 
+	return false;
+label_write_error:
+	prof_dump_close(propagate_err);
+	return true;
+}
+
+static bool
+prof_dump(tsd_t *tsd, bool propagate_err, const char *filename, bool leakcheck)
+{
+	prof_tdata_t *tdata;
+	struct prof_tdata_merge_iter_arg_s prof_tdata_merge_iter_arg;
+	struct prof_gctx_merge_iter_arg_s prof_gctx_merge_iter_arg;
+	struct prof_gctx_dump_iter_arg_s prof_gctx_dump_iter_arg;
+	prof_gctx_tree_t gctxs;
+	bool err;
+
+	cassert(config_prof);
+
+	tdata = prof_tdata_get(tsd, true);
+	if (tdata == NULL) {
+		return true;
+	}
+
+	malloc_mutex_lock(tsd_tsdn(tsd), &prof_dump_mtx);
+
+	prof_dump_prep(tsd, tdata, &prof_tdata_merge_iter_arg,
+	    &prof_gctx_merge_iter_arg, &gctxs);
+	err = prof_dump_file(tsd, propagate_err, filename, leakcheck, tdata,
+	    &prof_tdata_merge_iter_arg, &prof_gctx_merge_iter_arg,
+	    &prof_gctx_dump_iter_arg, &gctxs);
 	prof_gctx_finish(tsd, &gctxs);
+
 	malloc_mutex_unlock(tsd_tsdn(tsd), &prof_dump_mtx);
 
+	if (err) {
+		return true;
+	}
+
 	if (leakcheck) {
 		prof_leakcheck(&prof_tdata_merge_iter_arg.cnt_all,
 		    prof_gctx_merge_iter_arg.leak_ngctx, filename);
 	}
-	return (false);
-label_write_error:
-	prof_dump_close(propagate_err);
-label_open_close_error:
-	prof_gctx_finish(tsd, &gctxs);
-	malloc_mutex_unlock(tsd_tsdn(tsd), &prof_dump_mtx);
-	return (true);
+	return false;
 }
 
+#ifdef JEMALLOC_JET
+void
+prof_cnt_all(uint64_t *curobjs, uint64_t *curbytes, uint64_t *accumobjs,
+    uint64_t *accumbytes)
+{
+	tsd_t *tsd;
+	prof_tdata_t *tdata;
+	struct prof_tdata_merge_iter_arg_s prof_tdata_merge_iter_arg;
+	struct prof_gctx_merge_iter_arg_s prof_gctx_merge_iter_arg;
+	prof_gctx_tree_t gctxs;
+
+	tsd = tsd_fetch();
+	tdata = prof_tdata_get(tsd, false);
+	if (tdata == NULL) {
+		if (curobjs != NULL) {
+			*curobjs = 0;
+		}
+		if (curbytes != NULL) {
+			*curbytes = 0;
+		}
+		if (accumobjs != NULL) {
+			*accumobjs = 0;
+		}
+		if (accumbytes != NULL) {
+			*accumbytes = 0;
+		}
+		return;
+	}
+
+	prof_dump_prep(tsd, tdata, &prof_tdata_merge_iter_arg,
+	    &prof_gctx_merge_iter_arg, &gctxs);
+	prof_gctx_finish(tsd, &gctxs);
+
+	if (curobjs != NULL) {
+		*curobjs = prof_tdata_merge_iter_arg.cnt_all.curobjs;
+	}
+	if (curbytes != NULL) {
+		*curbytes = prof_tdata_merge_iter_arg.cnt_all.curbytes;
+	}
+	if (accumobjs != NULL) {
+		*accumobjs = prof_tdata_merge_iter_arg.cnt_all.accumobjs;
+	}
+	if (accumbytes != NULL) {
+		*accumbytes = prof_tdata_merge_iter_arg.cnt_all.accumbytes;
+	}
+}
+#endif
+
 #define	DUMP_FILENAME_BUFSIZE	(PATH_MAX + 1)
 #define	VSEQ_INVALID		UINT64_C(0xffffffffffffffff)
 static void

test/unit/prof_tctx.c

@@ -0,0 +1,57 @@
+#include "test/jemalloc_test.h"
+
+#ifdef JEMALLOC_PROF
+const char *malloc_conf = "prof:true,lg_prof_sample:0";
+#endif
+
+TEST_BEGIN(test_prof_realloc)
+{
+	tsdn_t *tsdn;
+	int flags;
+	void *p, *q;
+	extent_t *extent_p, *extent_q;
+	prof_tctx_t *tctx_p, *tctx_q;
+	uint64_t curobjs_0, curobjs_1, curobjs_2, curobjs_3;
+
+	test_skip_if(!config_prof);
+
+	tsdn = tsdn_fetch();
+	flags = MALLOCX_TCACHE_NONE;
+
+	prof_cnt_all(&curobjs_0, NULL, NULL, NULL);
+	p = mallocx(1024, flags);
+	assert_ptr_not_null(p, "Unexpected mallocx() failure");
+	extent_p = iealloc(tsdn, p);
+	assert_ptr_not_null(extent_p, "Unexpected iealloc() failure");
+	tctx_p = prof_tctx_get(tsdn, extent_p, p);
+	assert_ptr_ne(tctx_p, (prof_tctx_t *)(uintptr_t)1U,
+	    "Expected valid tctx");
+	prof_cnt_all(&curobjs_1, NULL, NULL, NULL);
+	assert_u64_eq(curobjs_0 + 1, curobjs_1,
+	    "Allocation should have increased sample size");
+
+	q = rallocx(p, 2048, flags);
+	assert_ptr_ne(p, q, "Expected move");
+	assert_ptr_not_null(p, "Unexpected rmallocx() failure");
+	extent_q = iealloc(tsdn, q);
+	assert_ptr_not_null(extent_q, "Unexpected iealloc() failure");
+	tctx_q = prof_tctx_get(tsdn, extent_q, q);
+	assert_ptr_ne(tctx_q, (prof_tctx_t *)(uintptr_t)1U,
+	    "Expected valid tctx");
+	prof_cnt_all(&curobjs_2, NULL, NULL, NULL);
+	assert_u64_eq(curobjs_1, curobjs_2,
+	    "Reallocation should not have changed sample size");
+
+	dallocx(q, flags);
+	prof_cnt_all(&curobjs_3, NULL, NULL, NULL);
+	assert_u64_eq(curobjs_0, curobjs_3,
+	    "Sample size should have returned to base level");
+}
+TEST_END
+
+int
+main(void)
+{
+	return test(
+	    test_prof_realloc);
+}