Re: [PATCH v3 2/6] kernel-shark-qt: Introduce the visualization model used by the Qt-based KS

["Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx>]

On  4.08.2018 00:48, Steven Rostedt wrote:
> On Fri,  3 Aug 2018 17:29:33 +0300
> "Yordan Karadzhov (VMware)" <y.karadz@xxxxxxxxx> wrote:
>
> > --- /dev/null
> > +++ b/kernel-shark-qt/src/libkshark-model.h
> > @@ -0,0 +1,149 @@
> > +/* SPDX-License-Identifier: LGPL-2.1 */
> > +
> > +/*
> > + * Copyright (C) 2017 VMware Inc, Yordan Karadzhov <y.karadz@xxxxxxxxx>
> > + */
> > +
> > + /**
> > +  *  @file    libkshark-model.h
> > +  *  @brief   Visualization model for FTRACE (trace-cmd) data.
> > +  */
> > +
> > +#ifndef _LIB_KSHARK_MODEL_H
> > +#define _LIB_KSHARK_MODEL_H
> > +
> > +// KernelShark
> > +#include "libkshark.h"
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif // __cplusplus
> > +
> > +/**
> > + * Overflow Bin identifiers. The two overflow bins are used to hold the data
> > + * outside the visualized range.
> > + */
> > +enum OverflowBin {
> > +	/**
> > +	 * Identifier of the Upper Overflow Bin. This bin is used to hold the data
> > +	 * before (in time) the beginning of the visualized range.
> > +	 */
> > +	UPPER_OVERFLOW_BIN = -1,
> > +
> > +	/** Identifier of the Lower Overflow Bin. This bin is used to hold the data
> > +	 * after (in time) the end of the visualized range.*/
> > +	LOWER_OVERFLOW_BIN = -2,
>
> Wait, I thought the upper overflow bin was to store the data after the
> visualized range and the lower overnflow bin the time before?

Correct, I have to swap the tow descriptions.

> > +};
> > +
> > +/** Structure describing the current state of the visualization model. */
>
>
> > +static size_t ksmodel_set_lower_edge(struct kshark_trace_histo *histo)
> > +{
> > +	/*
> > +	 * Find the index of the first entry inside
> > +	 * the range (timestamp > min).
> > +	 */
this comment contains a mistake as well. It has to be:
	 /*
	  * Find the index of the first entry inside
	  * the range (timestamp >= min). Note that the
	  * value of min is considered inside the range.
	  */

> > +	ssize_t row = kshark_find_entry_by_time(histo->min,
> > +						histo->data,
> > +						0,
> > +						histo->data_size - 1);
> > +
> > +	assert(row != BSEARCH_ALL_SMALLER);
> > +
> > +	if (row == BSEARCH_ALL_GREATER || row == 0) {
>
> LOB(hist) is set by the lower row (which is less in time isn't it?)


If the data[0]->ts == histo->min, "row" will be equal to 0. If the 
data-sat starts after histo->min (in time) then "row" will be
equal to BSEARCH_ALL_GREATER. In both cases the LOB is empty.

> > +		/* Lower Overflow bin is empty. */
> > +		histo->map[LOB(histo)] = KS_EMPTY_BIN;
> > +		histo->bin_count[LOB(histo)] = 0;
> > +		row = 0;
> > +	} else {
> > +		/*
> > +		 * The first entry inside the range is not the first entry
> > +		 * of the dataset. This means that the Lower Overflow bin
> > +		 * contains data.
> > +		 */
> > +
> > +		/* Lower Overflow bin starts at "0". */
> > +		histo->map[LOB(histo)] = 0;
> > +
> > +		/*
> > +		 * The number of entries inside the Lower Overflow bin is
> > +		 * equal to the index of the first entry inside the range.
> > +		 */
> > +		histo->bin_count[LOB(histo)] = row;
> > +	}
> > +
> > +	/*
> > +	 * Now check if the first entry inside the range falls into the
> > +	 * first bin.
> > +	 */
> > +	if (histo->data[row]->ts < histo->min + histo->bin_size) {
> > +		/*
> > +		 * It is inside the first bin. Set the beginning
> > +		 * of the first bin.
> > +		 */
> > +		histo->map[0] = row;
> > +	} else {
> > +		/* The first bin is empty. */
> > +		histo->map[0] = KS_EMPTY_BIN;
> > +	}
> > +
> > +	return row;
> > +}
> > +
> > +static size_t ksmodel_set_upper_edge(struct kshark_trace_histo *histo)
> > +{
> > +	/*
> > +	 * Find the index of the first entry outside the range
> > +	 * (timestamp > max). Remember that kshark_find_entry_by_time returns
> > +	 * the first entry which is equal or greater than the reference time.
> > +	 */

I will change this comment as well:
	/*
	 * Find the index of the first entry outside the range
	 * (timestamp > max). Note that the value of max is considered
	 * inside the range. Remember that kshark_find_entry_by_time
	 * returns the first entry which is equal or greater than the
	 * reference time.
	 */

> > +	ssize_t row = kshark_find_entry_by_time(histo->max + 1,

Here we search for "histo->max + 1"

> > +						histo->data,
> > +						0,
> > +						histo->data_size - 1);
> > +
> > +	assert(row != BSEARCH_ALL_GREATER);
> > +
> > +	if (row == BSEARCH_ALL_SMALLER || row == histo->data_size - 1) {

And I have a bug here. it has to be:

	if (row == BSEARCH_ALL_SMALLER) {


I will send this patch again.
Thanks!
Yordan


> UOB(histo) is set by the highest row. Right?
>
> -- Steve
>
> > +		/* Upper Overflow bin is empty. */
> > +		histo->map[UOB(histo)] = KS_EMPTY_BIN;
> > +		histo->bin_count[UOB(histo)] = 0;
> > +	} else {
> > +		/*
> > +		 * The Upper Overflow bin contains data. Set its beginning
> > +		 * and the number of entries.
> > +		 */
> > +		histo->map[UOB(histo)] = row;
> > +		histo->bin_count[UOB(histo)] = histo->data_size - row;
> > +	}
> > +
> > +	return row;
> > +}
> > +