Re: [PATCH v2 09/11] vinyl: do not account zero dump bandwidth

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Fri, Sep 28, 2018 at 08:40:07PM +0300, Vladimir Davydov wrote:
> Since we free memory in 16 MB blocks (see SLAB_SIZE), it may occur that
> we dump almost all data stored in a block but still have to leave it be,
> because it contains data of a newer generation. If the memory limit is
> small (as it is typically in tests), this may result in dumping 0 bytes.
> In order not to disrupt statistics and throttling transactions in vain,
> let's simply ignore such results. Normally, the memory limit should be
> large enough for such granularity not to affect the measurements
> (hundreds of megabytes) so this problem isn't worth putting more efforts
> into.
> 
> Needed for #1862
> ---
>  src/box/vy_regulator.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/src/box/vy_regulator.c b/src/box/vy_regulator.c
> index 5ec5629f..682777fc 100644
> --- a/src/box/vy_regulator.c
> +++ b/src/box/vy_regulator.c
> @@ -200,7 +200,20 @@ vy_regulator_dump_complete(struct vy_regulator *regulator,
>  {
>  	regulator->dump_in_progress = false;
>  
> -	if (dump_duration > 0) {
> +	/*
> +	 * Update dump bandwidth.
> +	 *
> +	 * Note, since we free memory in 16 MB blocks (see SLAB_SIZE),
> +	 * it may occur that we dump almost all data stored in a block
> +	 * but still have to leave it be, because it contains data of
> +	 * a newer generation. If the memory limit is small, this may
> +	 * result in dumping 0 bytes. In order not to disrupt statistics
> +	 * let's simply ignore such results. Normally, the memory limit
> +	 * should be large enough for such granularity not to affect the
> +	 * measurements (hundreds of megabytes) so this problem isn't
> +	 * worth putting more efforts into.
> +	 */
> +	if (mem_dumped > 0 && dump_duration > 0) {
>  		histogram_collect(regulator->dump_bw_hist,
>  				  mem_dumped / dump_duration);
>  		/*

Turns out this isn't enough. We'd better not account too small dumps,
because such dumps have too high overhead associated with file creation.
Our tests create a lot of small dumps using box.snapshot(). Taking them
into account may slow down the overall test execution time or even break
some tests. Let's ignore all dumps of size less than 1 MB for bandwidth
estimation. The new patch is below.

From 8d5858835c61ed290e753241e146cee50c41e6db Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Date: Fri, 12 Oct 2018 16:12:06 +0300
Subject: [PATCH] vinyl: do not account small dumps for bandwidth estimation

Small dumps (e.g. triggered by box.snapshot) have too high overhead
associated with file creation so taking them into account for bandwidth
estimation may result in erroneous transaction throttling. Let's ignore
dumps of size less than 1 MB.

Needed for #1862

diff --git a/src/box/vy_regulator.c b/src/box/vy_regulator.c
index 6ecb5aa6..c6a56905 100644
--- a/src/box/vy_regulator.c
+++ b/src/box/vy_regulator.c
@@ -66,6 +66,13 @@ static const int VY_DUMP_BANDWIDTH_PCT = 10;
  */
 static const size_t VY_DUMP_BANDWIDTH_DEFAULT = 10 * 1024 * 1024;
 
+/**
+ * Do not take into account small dumps when estimating dump
+ * bandwidth, because they have too high overhead associated
+ * with file creation.
+ */
+static const size_t VY_DUMP_SIZE_ACCT_MIN = 1024 * 1024;
+
 static void
 vy_regulator_trigger_dump(struct vy_regulator *regulator)
 {
@@ -205,7 +212,7 @@ vy_regulator_dump_complete(struct vy_regulator *regulator,
 {
 	regulator->dump_in_progress = false;
 
-	if (dump_duration > 0) {
+	if (mem_dumped >= VY_DUMP_SIZE_ACCT_MIN && dump_duration > 0) {
 		histogram_collect(regulator->dump_bandwidth_hist,
 				  mem_dumped / dump_duration);
 		/*