[PATCH 1/3] kernel-shark: Simplify the way collections handle data requests

[PATCH 1/3] kernel-shark: Simplify the way collections handle data requests

[Yordan Karadzhov (VMware)]

This patch initially intended to fix one potential memory leak in
libkshark-collection. However, as pointed out by Steven in his review
of the previous version, there is no need to carrying the address of
the pointer of the original data requests trough the entire procedure
of searching for a data entry. Instead we can always use copies of the
pointer and rely on the fact that the entire list of data requests will
be anyway freed at the end, regardless of the success or the failure of
the search.

Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/libkshark-collection.c | 114 ++++++++++++------------
 kernel-shark/src/libkshark-model.c      |  10 +--
 kernel-shark/src/libkshark.h            |   4 +-
 3 files changed, 62 insertions(+), 66 deletions(-)

diff --git a/kernel-shark/src/libkshark-collection.c b/kernel-shark/src/libkshark-collection.c
index 02a014e..66cdbca 100644
--- a/kernel-shark/src/libkshark-collection.c
+++ b/kernel-shark/src/libkshark-collection.c
@@ -308,29 +308,28 @@ map_collection_index_from_source(const struct kshark_entry_collection *col,
 
 static ssize_t
 map_collection_request_init(const struct kshark_entry_collection *col,
-			    struct kshark_entry_request **req,
+			    struct kshark_entry_request *req,
 			    bool front, size_t *end)
 {
-	struct kshark_entry_request *req_tmp = *req;
 	int col_index_flag;
 	ssize_t col_index;
 	size_t req_end;
 
-	if (req_tmp->next || col->size == 0) {
+	if (req->next || col->size == 0) {
 		fprintf(stderr, "Unexpected input in ");
 		fprintf(stderr, "map_collection_request_init()\n");
 		goto do_nothing;
 	}
 
-	req_end = front ? req_tmp->first + req_tmp->n - 1 :
-			  req_tmp->first - req_tmp->n + 1;
+	req_end = front ? req->first + req->n - 1 :
+			  req->first - req->n + 1;
 
 	/*
 	 * Find the first Resume Point of the collection which is equal or
 	 * greater than the first index of this request.
 	 */
 	col_index = map_collection_index_from_source(col,
-						     req_tmp->first,
+						     req->first,
 						     &col_index_flag);
 
 	/*
@@ -370,8 +369,8 @@ map_collection_request_init(const struct kshark_entry_collection *col,
 		 * going backwards, so the proper place to start will be the
 		 * Break point of "col_index".
 		 */
-		req_tmp->first = front ? col->resume_points[++col_index] :
-					 col->break_points[col_index];
+		req->first = front ? col->resume_points[++col_index] :
+				     col->break_points[col_index];
 	}
 
 	if (col_index_flag == COLLECTION_BEFORE) {
@@ -403,8 +402,8 @@ map_collection_request_init(const struct kshark_entry_collection *col,
 		 * backwards, so the proper place to start will be the Break
 		 * point of "col_index - 1".
 		 */
-		req_tmp->first = front ? col->resume_points[col_index] :
-					 col->break_points[--col_index];
+		req->first = front ? col->resume_points[col_index] :
+				     col->break_points[--col_index];
 	}
 
 	*end = req_end;
@@ -412,8 +411,6 @@ map_collection_request_init(const struct kshark_entry_collection *col,
 	return col_index;
 
 do_nothing:
-	kshark_free_entry_request(*req);
-	*req = NULL;
 	*end = KS_EMPTY_BIN;
 
 	return KS_EMPTY_BIN;
@@ -426,9 +423,8 @@ do_nothing:
  */
 static int
 map_collection_back_request(const struct kshark_entry_collection *col,
-			    struct kshark_entry_request **req)
+			    struct kshark_entry_request *req)
 {
-	struct kshark_entry_request *req_tmp;
 	size_t req_first, req_end;
 	ssize_t col_index;
 	int req_count;
@@ -442,7 +438,6 @@ map_collection_back_request(const struct kshark_entry_collection *col,
 	 * the end of the inputted request and create a separate request for
 	 * each of those interest.
 	 */
-	req_tmp = *req;
 	req_count = 1;
 	while (col_index >= 0 && req_end <= col->break_points[col_index]) {
 		if (req_end >= col->resume_points[col_index]) {
@@ -451,7 +446,7 @@ map_collection_back_request(const struct kshark_entry_collection *col,
 			 * the "col_index" collection interval. Close the
 			 * collection request here and return.
 			 */
-			req_tmp->n = req_tmp->first - req_end + 1;
+			req->n = req->first - req_end + 1;
 			break;
 		}
 
@@ -461,8 +456,8 @@ map_collection_back_request(const struct kshark_entry_collection *col,
 		 * end of this interval and move to the next one. Try to make
 		 * another request there.
 		 */
-		req_tmp->n = req_tmp->first -
-		             col->resume_points[col_index] + 1;
+		req->n = req->first -
+			 col->resume_points[col_index] + 1;
 
 		--col_index;
 
@@ -478,18 +473,18 @@ map_collection_back_request(const struct kshark_entry_collection *col,
 			/* Make a new request. */
 			req_first = col->break_points[col_index];
 
-			req_tmp->next =
+			req->next =
 				kshark_entry_request_alloc(req_first,
 							   0,
-							   req_tmp->cond,
-							   req_tmp->val,
-							   req_tmp->vis_only,
-							   req_tmp->vis_mask);
+							   req->cond,
+							   req->val,
+							   req->vis_only,
+							   req->vis_mask);
 
-			if (!req_tmp->next)
+			if (!req->next)
 				goto fail;
 
-			req_tmp = req_tmp->next;
+			req = req->next;
 			++req_count;
 		}
 	}
@@ -497,10 +492,9 @@ map_collection_back_request(const struct kshark_entry_collection *col,
 	return req_count;
 
 fail:
-	fprintf(stderr, "Failed to allocate memory for ");
-	fprintf(stderr, "Collection data request.\n");
-	kshark_free_entry_request(*req);
-	*req = NULL;
+	fprintf(stderr,
+		"Failed to allocate memory for Collection data request.\n");
+
 	return -ENOMEM;
 }
 
@@ -511,9 +505,8 @@ fail:
  */
 static int
 map_collection_front_request(const struct kshark_entry_collection *col,
-			     struct kshark_entry_request **req)
+			     struct kshark_entry_request *req)
 {
-	struct kshark_entry_request *req_tmp;
 	size_t req_first, req_end;
 	ssize_t col_index;
 	int req_count;
@@ -528,7 +521,6 @@ map_collection_front_request(const struct kshark_entry_collection *col,
 	 * each of those interest.
 	 */
 	req_count = 1;
-	req_tmp = *req;
 	while (col_index < col->size &&
 	       req_end >= col->resume_points[col_index]) {
 		if (req_end <= col->break_points[col_index]) {
@@ -537,7 +529,7 @@ map_collection_front_request(const struct kshark_entry_collection *col,
 			 * the "col_index" collection interval.
 			 * Close the collection request here and return.
 			 */
-			req_tmp->n = req_end - req_tmp->first + 1;
+			req->n = req_end - req->first + 1;
 			break;
 		}
 
@@ -547,8 +539,8 @@ map_collection_front_request(const struct kshark_entry_collection *col,
 		 * request at the end of the interval and move to the next
 		 * interval. Try to make another request there.
 		 */
-		req_tmp->n = col->break_points[col_index] -
-			     req_tmp->first + 1;
+		req->n = col->break_points[col_index] -
+			 req->first + 1;
 
 		++col_index;
 
@@ -565,18 +557,18 @@ map_collection_front_request(const struct kshark_entry_collection *col,
 			/* Make a new request. */
 			req_first = col->resume_points[col_index];
 
-			req_tmp->next =
+			req->next =
 				kshark_entry_request_alloc(req_first,
 							   0,
-							   req_tmp->cond,
-							   req_tmp->val,
-							   req_tmp->vis_only,
-							   req_tmp->vis_mask);
+							   req->cond,
+							   req->val,
+							   req->vis_only,
+							   req->vis_mask);
 
-			if (!req_tmp->next)
+			if (!req->next)
 				goto fail;
 
-			req_tmp = req_tmp->next;
+			req = req->next;
 			++req_count;
 		}
 	}
@@ -584,10 +576,9 @@ map_collection_front_request(const struct kshark_entry_collection *col,
 	return req_count;
 
 fail:
-	fprintf(stderr, "Failed to allocate memory for ");
-	fprintf(stderr, "Collection data request.\n");
-	kshark_free_entry_request(*req);
-	*req = NULL;
+	fprintf(stderr,
+		"Failed to allocate memory for Collection data request.\n");
+
 	return -ENOMEM;
 }
 
@@ -616,7 +607,7 @@ fail:
  *	    "Dummy entry".
  */
 const struct kshark_entry *
-kshark_get_collection_entry_front(struct kshark_entry_request **req,
+kshark_get_collection_entry_front(struct kshark_entry_request *req,
 				  struct kshark_entry **data,
 				  const struct kshark_entry_collection *col,
 				  ssize_t *index)
@@ -624,26 +615,28 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
 	const struct kshark_entry *entry = NULL;
 	int req_count;
 
+	if (index)
+		*index = KS_EMPTY_BIN;
+
 	/*
 	 * Use the intervals of the Data collection to redefine the data
 	 * request in a way which will ignore the data outside of the
 	 * intervals of the collection.
 	 */
 	req_count = map_collection_front_request(col, req);
-
-	if (index && !req_count)
-		*index = KS_EMPTY_BIN;
+	if (req_count <= 0)
+		return NULL;
 
 	/*
 	 * Loop over the list of redefined requests and search until you find
 	 * the first matching entry.
 	 */
-	while (*req) {
-		entry = kshark_get_entry_front(*req, data, index);
+	while (req) {
+		entry = kshark_get_entry_front(req, data, index);
 		if (entry)
 			break;
 
-		*req = (*req)->next;
+		req = req->next;
 	}
 
 	return entry;
@@ -674,7 +667,7 @@ kshark_get_collection_entry_front(struct kshark_entry_request **req,
  *	    "Dummy entry".
  */
 const struct kshark_entry *
-kshark_get_collection_entry_back(struct kshark_entry_request **req,
+kshark_get_collection_entry_back(struct kshark_entry_request *req,
 				 struct kshark_entry **data,
 				 const struct kshark_entry_collection *col,
 				 ssize_t *index)
@@ -682,25 +675,28 @@ kshark_get_collection_entry_back(struct kshark_entry_request **req,
 	const struct kshark_entry *entry = NULL;
 	int req_count;
 
+	if (index)
+		*index = KS_EMPTY_BIN;
+
 	/*
 	 * Use the intervals of the Data collection to redefine the data
 	 * request in a way which will ignore the data outside of the
 	 * intervals of the collection.
 	 */
 	req_count = map_collection_back_request(col, req);
-	if (index && !req_count)
-		*index = KS_EMPTY_BIN;
+	if (req_count <= 0)
+		return NULL;
 
 	/*
 	 * Loop over the list of redefined requests and search until you find
 	 * the first matching entry.
 	 */
-	while (*req) {
-		entry = kshark_get_entry_back(*req, data, index);
+	while (req) {
+		entry = kshark_get_entry_back(req, data, index);
 		if (entry)
 			break;
 
-		*req = (*req)->next;
+		req = req->next;
 	}
 
 	return entry;
diff --git a/kernel-shark/src/libkshark-model.c b/kernel-shark/src/libkshark-model.c
index 6c54e1e..babac2a 100644
--- a/kernel-shark/src/libkshark-model.c
+++ b/kernel-shark/src/libkshark-model.c
@@ -918,7 +918,7 @@ ksmodel_get_entry_front(struct kshark_trace_histo *histo,
 		return NULL;
 
 	if (col && col->size)
-		entry = kshark_get_collection_entry_front(&req, histo->data,
+		entry = kshark_get_collection_entry_front(req, histo->data,
 							  col, index);
 	else
 		entry = kshark_get_entry_front(req, histo->data, index);
@@ -965,8 +965,8 @@ ksmodel_get_entry_back(struct kshark_trace_histo *histo,
 		return NULL;
 
 	if (col && col->size)
-		entry = kshark_get_collection_entry_back(&req, histo->data,
-							  col, index);
+		entry = kshark_get_collection_entry_back(req, histo->data,
+							 col, index);
 	else
 		entry = kshark_get_entry_back(req, histo->data, index);
 
@@ -1178,7 +1178,7 @@ bool ksmodel_cpu_visible_event_exist(struct kshark_trace_histo *histo,
 	req->vis_mask = KS_EVENT_VIEW_FILTER_MASK;
 
 	if (col && col->size)
-		entry = kshark_get_collection_entry_front(&req, histo->data,
+		entry = kshark_get_collection_entry_front(req, histo->data,
 							  col, index);
 	else
 		entry = kshark_get_entry_front(req, histo->data, index);
@@ -1231,7 +1231,7 @@ bool ksmodel_task_visible_event_exist(struct kshark_trace_histo *histo,
 	req->vis_mask = KS_EVENT_VIEW_FILTER_MASK;
 
 	if (col && col->size)
-		entry = kshark_get_collection_entry_front(&req, histo->data,
+		entry = kshark_get_collection_entry_front(req, histo->data,
 							  col, index);
 	else
 		entry = kshark_get_entry_front(req, histo->data, index);
diff --git a/kernel-shark/src/libkshark.h b/kernel-shark/src/libkshark.h
index 3407db1..3b027c2 100644
--- a/kernel-shark/src/libkshark.h
+++ b/kernel-shark/src/libkshark.h
@@ -445,13 +445,13 @@ void kshark_reset_data_collection(struct kshark_entry_collection *col);
 void kshark_free_collection_list(struct kshark_entry_collection *col);
 
 const struct kshark_entry *
-kshark_get_collection_entry_front(struct kshark_entry_request **req,
+kshark_get_collection_entry_front(struct kshark_entry_request *req,
 				  struct kshark_entry **data,
 				  const struct kshark_entry_collection *col,
 				  ssize_t *index);
 
 const struct kshark_entry *
-kshark_get_collection_entry_back(struct kshark_entry_request **req,
+kshark_get_collection_entry_back(struct kshark_entry_request *req,
 				 struct kshark_entry **data,
 				 const struct kshark_entry_collection *col,
 				 ssize_t *index);
-- 
2.20.1

[PATCH 2/3] kernel-shark: Avoid redrawing the graphs when switching the keyboard focus

[Yordan Karadzhov (VMware)]

Reimplementing the event handler of the focus event, in order to avoid
the update (redrawing) of the graphs every time when the OpenGL widget
grabs / releases the focus of the keyboard. This is done because we do
not need to redraw, while on the other hand on large data-sets, redrawing
can take a lot of time.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsGLWidget.hpp | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/kernel-shark/src/KsGLWidget.hpp b/kernel-shark/src/KsGLWidget.hpp
index 3d428b1..c6fd787 100644
--- a/kernel-shark/src/KsGLWidget.hpp
+++ b/kernel-shark/src/KsGLWidget.hpp
@@ -64,6 +64,24 @@ public:
 
 	void loadColors();
 
+	/**
+	 * Reimplementing the event handler of the focus event, in order to
+	 * avoid the update (redrawing) of the graphs every time when the
+	 * widget grabs the focus of the keyboard. This is done because we do
+	 * not need to redraw, while on the other hand on large data-sets,
+	 * redrawing can take a lot of time.
+	 */
+	void focusInEvent(QFocusEvent* e) override {}
+
+	/**
+	 * Reimplementing the event handler of the focus event, in order to
+	 * avoid the update (redrawing) of the graphs every time when the
+	 * widget releases the focus of the keyboard. This is done because we
+	 * do not need to redraw, while on the other hand on large data-sets,
+	 * redrawing can take a lot of time.
+	 */
+	void focusOutEvent(QFocusEvent* e) override {}
+
 	/**
 	 * Provide the widget with a pointer to the Dual Marker state machine
 	 * object.
-- 
2.20.1

[PATCH 3/3] kernel-shark: Search for visible entry only if this is really needed

[Yordan Karadzhov (VMware)]

ksmodel_task_visible_event_exist() and ksmodel_cpu_visible_event_exist()
are relatively expensive operations. It make sense to perform those only
if the entry that was found so far has been filtered out (is invisible).
The patch also makes the processing of the CPU and Task graphs more
consistent.

Signed-off-by: Yordan Karadzhov (VMware) <y.karadz@gmail.com>
---
 kernel-shark/src/KsPlotTools.cpp | 42 ++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 15 deletions(-)

diff --git a/kernel-shark/src/KsPlotTools.cpp b/kernel-shark/src/KsPlotTools.cpp
index a8eddcd..fe3008e 100644
--- a/kernel-shark/src/KsPlotTools.cpp
+++ b/kernel-shark/src/KsPlotTools.cpp
@@ -829,14 +829,17 @@ void Graph::fillCPUGraph(int cpu)
 			pidBack = KS_FILTERED_BIN;
 
 		visMask = 0x0;
-		if (ksmodel_cpu_visible_event_exist(_histoPtr, bin,
-							       cpu,
-							       _collectionPtr,
-							       &index))
-
-			visMask = _histoPtr->data[index]->visible;
-		else if (eFront)
-			visMask = eFront->visible;
+		if (eFront) {
+			if (!(eFront->visible & KS_EVENT_VIEW_FILTER_MASK) &&
+			    ksmodel_cpu_visible_event_exist(_histoPtr, bin,
+								       cpu,
+								       _collectionPtr,
+								       &index)) {
+				visMask = _histoPtr->data[index]->visible;
+			} else {
+				visMask = eFront->visible;
+			}
+		}
 	};
 
 	auto lamSetBin = [&] (int bin)
@@ -918,6 +921,7 @@ void Graph::fillCPUGraph(int cpu)
 void Graph::fillTaskGraph(int pid)
 {
 	int cpuFront, cpuBack(0), pidFront(0), pidBack(0), lastCpu(-1), bin(0);
+	struct kshark_entry *eFront;
 	uint8_t visMask;
 	ssize_t index;
 
@@ -991,12 +995,15 @@ void Graph::fillTaskGraph(int pid)
 
 	auto lamGetPidCPU = [&] (int bin)
 	{
+		eFront = nullptr;
 		/* Get the CPU used by this task. */
 		cpuFront = ksmodel_get_cpu_front(_histoPtr, bin,
 						 pid,
 						 false,
 						 _collectionPtr,
-						 nullptr);
+						 &index);
+		if (index >= 0)
+			eFront = _histoPtr->data[index];
 
 		cpuBack = ksmodel_get_cpu_back(_histoPtr, bin,
 					       pid,
@@ -1026,12 +1033,17 @@ void Graph::fillTaskGraph(int pid)
 						       nullptr);
 
 			visMask = 0x0;
-			if (ksmodel_task_visible_event_exist(_histoPtr,
-							     bin,
-							     pid,
-							     _collectionPtr,
-							     &index)) {
-				visMask = _histoPtr->data[index]->visible;
+			if (eFront) {
+				if (!(eFront->visible & KS_EVENT_VIEW_FILTER_MASK) &&
+				    ksmodel_task_visible_event_exist(_histoPtr,
+								     bin,
+								     pid,
+								     _collectionPtr,
+								     &index)) {
+					visMask = _histoPtr->data[index]->visible;
+				} else {
+					visMask = eFront->visible;
+				}
 			}
 		}
 	};
-- 
2.20.1