Wireshark · Wireshark-dev: Re: [Wireshark-dev] New packet list: Optimize memory usage

[Jakub Zawadzki <darkjames@xxxxxxxxxxxxxxxx>]

On Sun, Jul 12, 2009 at 09:52:49PM -0600, Stephen Fisher wrote:
> I experimented with re-dissecting a  while back when I first started one 
> of my versions of the new packet list and saw significant CPU utilization
> because whenever you even drag the mouse arrow up/down along the displayed
> columns, it is constantly updating (when using data cell functions at least).

Yeah, that's why it'd be nice if we have some kind of cache.
I experiment today with gtk_tree_view_get_visible_range()
and removing "old" columns data - It's working quite nice.

Btw. While scrolling packet list I monitor memory usage,
it's seems like there's some memory leak. I run valgrind, but I can't see any.

diff --git file.c file.c
index 71ac4d1..181c32f 100644
--- file.c
+++ file.c
@@ -1046,7 +1046,7 @@ add_packet_to_packet_list(frame_data *fdata, capture_file *cf,
      evaluated. */
   if ((dfcode != NULL && refilter) || color_filters_used() ||
       filtering_tap_listeners || (tap_flags & TL_REQUIRES_PROTO_TREE) ||
-      have_custom_cols(&cf->cinfo))
+      0 /* have_custom_cols(&cf->cinfo) */)
 	  create_proto_tree = TRUE;
 
   /* Dissect the frame. */
@@ -1060,7 +1060,7 @@ add_packet_to_packet_list(frame_data *fdata, capture_file *cf,
       color_filters_prime_edt(edt);
   }
 
-  col_custom_prime_edt(edt, &cf->cinfo);
+  /* col_custom_prime_edt(edt, &cf->cinfo); */
 
   tap_queue_init(edt);
   epan_dissect_run(edt, pseudo_header, buf, fdata, &cf->cinfo);
@@ -1090,7 +1090,7 @@ add_packet_to_packet_list(frame_data *fdata, capture_file *cf,
       cum_bytes += fdata->pkt_len;
     }
 
-    epan_dissect_fill_in_columns(edt);
+    /* epan_dissect_fill_in_columns(edt); */	/* dynamic */
 
     /* If we haven't yet seen the first frame, this is it.
 
diff --git gtk/new_packet_list.c gtk/new_packet_list.c
index 862f039..0134d4e 100644
--- gtk/new_packet_list.c
+++ gtk/new_packet_list.c
@@ -87,21 +87,10 @@ new_packet_list_create(void)
 }
 
 guint
-new_packet_list_append(column_info cinfo, frame_data *fdata)
+new_packet_list_append(column_info cinfo _U_, frame_data *fdata)
 {
-	gint i;
 	row_data_t row_data;
 
-	/* Allocate the array holding column data, the size is the current number of columns */
-	row_data.col_text = se_alloc0(sizeof(row_data.col_text)*cfile.cinfo.num_cols);
-	row_data.col_fmt = (gint *) se_alloc(sizeof(gint) * cfile.cinfo.num_cols);
-
-	for(i = 0; i < cfile.cinfo.num_cols; i++) {
-		row_data.col_text[i] =
-			se_strdup(cinfo.col_data[i]);
-		row_data.col_fmt[i] = cinfo.col_fmt[i];
-	}
-
 	row_data.fdata = fdata;
 
 	packet_list_append_record(packetlist, &row_data);
diff --git gtk/packet_list_store.c gtk/packet_list_store.c
index 07a4d09..7bed2be 100644
--- gtk/packet_list_store.c
+++ gtk/packet_list_store.c
@@ -203,24 +203,6 @@ packet_list_tree_model_init(GtkTreeModelIface *iface)
 static void
 packet_list_init(PacketList *packet_list)
 {
-	guint i;
-	gint fmt;
-
-	for(i = 0; i < (guint)cfile.cinfo.num_cols; i++) {
-		/* Get the format of the column, see column_info.h */
-		fmt = get_column_format(i);
-		switch(fmt){
-			/* if we wish to store data rater than strings for some
-			 * colum types add case statements to the switch.
-			 */
-			case COL_NUMBER:
-			default:
-				packet_list->column_types[i] = G_TYPE_STRING;
-				break;
-		}
-	}
-	
-	packet_list->n_columns = (guint)cfile.cinfo.num_cols;
 	packet_list->num_rows = 0;
 	packet_list->rows = NULL;
 
@@ -229,6 +211,8 @@ packet_list_init(PacketList *packet_list)
 
 	packet_list->stamp = g_random_int(); /* To check whether an iter belongs
 					      * to our model. */
+
+	packet_list->cached = NULL;
 }
 
 /* This function is called just before a packet list is destroyed.  Free
@@ -258,17 +242,17 @@ packet_list_get_n_columns(GtkTreeModel *tree_model)
 {
 	g_return_val_if_fail(PACKETLIST_IS_LIST(tree_model), 0);
 
-	return PACKET_LIST(tree_model)->n_columns;
+	return cfile.cinfo.num_cols;
 }
 
 static GType
 packet_list_get_column_type(GtkTreeModel *tree_model, gint index)
 {
 	g_return_val_if_fail(PACKETLIST_IS_LIST(tree_model), G_TYPE_INVALID);
-	g_return_val_if_fail(index < PACKET_LIST(tree_model)->n_columns &&
+	g_return_val_if_fail(index < cfile.cinfo.num_cols &&
 			     index >= 0, G_TYPE_INVALID);
 
-	return PACKET_LIST(tree_model)->column_types[index];
+	return G_TYPE_STRING;	/* always string */
 }
 
 static gboolean
@@ -331,6 +315,140 @@ packet_list_get_path(GtkTreeModel *tree_model, GtkTreeIter *iter)
 	return path;
 }
 
+static const gchar **
+packet_list_record_get_columns(PacketListRecord *record)
+{
+	capture_file *cf = &cfile;
+	epan_dissect_t *edt;
+
+	gchar *err_info;
+	int err;
+
+#if 0	/* doesn't work, dunno how */
+	if (cf->current_frame == record->fdata) {
+		printf("hit cache: %p %p\n", cf->current_frame, record->fdata);
+		return cf->cinfo.col_data;
+	}
+#endif
+
+	if (!wtap_seek_read(cf->wth, record->fdata->file_off, &cf->pseudo_header,
+				cf->pd, record->fdata->cap_len, &err, &err_info))
+	{
+		return NULL;
+	}
+
+	edt = epan_dissect_new(have_custom_cols(&cf->cinfo), FALSE);
+	col_custom_prime_edt(edt, &cf->cinfo);
+	epan_dissect_run(edt, &cf->pseudo_header, cf->pd, record->fdata, &cf->cinfo);
+	epan_dissect_fill_in_columns(edt);
+	epan_dissect_free(edt);
+
+  	return cf->cinfo.col_data;
+}
+
+static const gchar *
+packet_list_record_get_column(PacketListRecord *record, gint column)
+{
+	const gchar **columns;
+
+	g_return_val_if_fail(column >= 0 && column < cfile.cinfo.num_cols, NULL);
+
+	if ((columns = packet_list_record_get_columns(record)))
+		return columns[column];
+
+	return "[ERROR]";
+}
+
+static void
+packet_list_cache_free(PacketList *packet_list, PacketListRecord *record)
+{
+	int i;
+
+	for (i = 0; i < cfile.cinfo.num_cols; i++)
+		g_free(record->col_text[i]);
+	g_free(record->col_text);
+	record->col_text = NULL;
+
+	packet_list->cached = g_slist_remove(packet_list->cached, record);
+}
+
+static guint
+packet_list_cache_remove_list(PacketList *packet_list, GSList *list)
+{
+	guint count = 0;
+	GSList *item;
+
+	for (item = packet_list->cached; item;) {
+		PacketListRecord *record = item->data;
+
+		item = g_slist_next(item);
+
+		if (g_slist_index(list, record) == -1) {
+			packet_list_cache_free(packet_list, record);
+			count++;
+		}
+	}
+	return count;
+}
+
+static guint
+packet_list_cache_remove_old(GtkTreeModel *model)
+{
+	PacketList *packet_list = PACKET_LIST(model);
+	guint count = 0;
+
+	GtkTreePath *start = NULL, *end = NULL;
+
+	if (gtk_tree_view_get_visible_range(GTK_TREE_VIEW(packet_list->view), &start, &end)) {
+		GSList *ilist = NULL;
+
+		do {
+			GtkTreeIter iter;
+
+			if (gtk_tree_model_get_iter(model, &iter, start))
+				ilist = g_slist_prepend(ilist, iter.user_data);
+			gtk_tree_path_next(start);
+
+		} while (gtk_tree_path_compare(start, end) != 0);
+
+		count = packet_list_cache_remove_list(packet_list, ilist);
+		g_slist_free(ilist);
+	}
+
+	gtk_tree_path_free(start);
+	gtk_tree_path_free(end);
+
+	return count;
+}
+
+static const gchar *
+packet_list_cache_get(GtkTreeModel *model, PacketListRecord *record, int column)
+{
+	PacketList *packet_list = PACKET_LIST(model);
+	const gchar **columns_data;
+	int i;
+
+	if (!record->col_text && 
+		(columns_data = packet_list_record_get_columns(record))) 
+	{
+		if (g_slist_length(packet_list->cached) > 30)
+			packet_list_cache_remove_old(model);	/* clear cache for items currently not visible */
+
+		record->col_text = g_new(char *, cfile.cinfo.num_cols);
+		packet_list->cached = g_slist_prepend(packet_list->cached, record);
+
+		/* printf("packet-list-cached-len: %u\n", g_slist_length(packet_list->cached)); */
+
+		for (i = 0; i < cfile.cinfo.num_cols; i++)
+			record->col_text[i] = g_strdup(columns_data[i]);
+	}
+
+	if (!record->col_text)
+		return "ERROR";
+
+	return record->col_text[column];
+}
+
 static void
 packet_list_get_value(GtkTreeModel *tree_model, GtkTreeIter *iter, gint column,
 		      GValue *value)
@@ -340,9 +458,9 @@ packet_list_get_value(GtkTreeModel *tree_model, GtkTreeIter *iter, gint column,
 
 	g_return_if_fail(PACKETLIST_IS_LIST(tree_model));
 	g_return_if_fail(iter != NULL);
-	g_return_if_fail(column < PACKET_LIST(tree_model)->n_columns);
+	g_return_if_fail(column >= 0 && column < cfile.cinfo.num_cols);
 
-	g_value_init(value, PACKET_LIST(tree_model)->column_types[column]);
+	g_value_init(value, G_TYPE_STRING);
 
 	packet_list = PACKET_LIST(tree_model);
 
@@ -351,7 +469,7 @@ packet_list_get_value(GtkTreeModel *tree_model, GtkTreeIter *iter, gint column,
 	if(record->pos >= packet_list->num_rows)
 		g_return_if_reached();
 
-	g_value_set_string(value, record->col_text[column]);
+	g_value_set_string(value, packet_list_cache_get(tree_model, record, column));
 }
 
 /* Takes an iter structure and sets it to point to the next row. */
@@ -496,7 +614,6 @@ packet_list_append_record(PacketList *packet_list, row_data_t *row_data)
 	GtkTreePath *path;
 	PacketListRecord *newrecord;
 	guint pos;
-	gint i;
 
 	g_return_if_fail(PACKETLIST_IS_LIST(packet_list));
 
@@ -507,18 +624,11 @@ packet_list_append_record(PacketList *packet_list, row_data_t *row_data)
  	packet_list->rows = g_renew(PacketListRecord*, packet_list->rows,
 				    packet_list->num_rows);
 
-	newrecord = se_alloc0(sizeof(PacketListRecord));
-	newrecord->col_text = se_alloc0(sizeof(row_data->col_text)* cfile.cinfo.num_cols);
-
-
-	/* XXX newrecord->col_text still uses the fmt index */
-	for(i = 0; i < cfile.cinfo.num_cols; i++)
-		newrecord->col_text[i] = row_data->col_text[i];
-
+	newrecord        = se_alloc(sizeof(PacketListRecord));
 	newrecord->fdata = row_data->fdata;
+	newrecord->pos   = pos;
 
 	packet_list->rows[pos] = newrecord;
-	newrecord->pos = pos;
 
 	/* Inform the tree view and other interested objects (such as tree row
 	 * references) that we have inserted a new row and where it was
@@ -623,8 +733,8 @@ packet_list_compare_records(gint sort_id, PacketListRecord *a,
 	 * seen by the user, whereas the col_text array from a and b is a
 	 * column format number. This matches them to each other to get
 	 * the right column text. */
-	gchar *a_col_text = a->col_text[cfile.cinfo.col_fmt[sort_id]];
-	gchar *b_col_text = b->col_text[cfile.cinfo.col_fmt[sort_id]];
+	const gchar *a_col_text = packet_list_record_get_column(a, sort_id);
+	const gchar *b_col_text = packet_list_record_get_column(b, sort_id);
 
 	if((a_col_text) && (b_col_text))
 		return strcmp(a_col_text, b_col_text);
diff --git gtk/packet_list_store.h gtk/packet_list_store.h
index 3262ac6..26df261 100644
--- gtk/packet_list_store.h
+++ gtk/packet_list_store.h
@@ -38,9 +38,6 @@
 #define PACKETLIST_LIST_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS((obj), PACKETLIST_TYPE_LIST, PacketListClass))
 
 typedef struct {
-	gint *col_fmt;
-	gchar **col_text;
-	gchar **col_filter;
 	frame_data *fdata;
 } row_data_t;
 
@@ -53,7 +50,7 @@ struct _PacketListRecord
 
 {
 	frame_data *fdata;
-	gchar **col_text;
+	gchar **col_text;	/* XXX, col_text _needs_ realloc() when column count change */
 
 	/* admin stuff used by the custom list model */
 	guint pos; /* position within the array */
@@ -69,10 +66,10 @@ struct _PacketList
 				  * the PacketListRecord structure for each
 				  * row. */
 
-	gint n_columns;
-	GType column_types[NUM_COL_FMTS];
 	GtkWidget *view; /* XXX - Does this really belong here?? */
 
+	GSList *cached;	/* list of cached records (column data) */
+
 	gint sort_id;
 	GtkSortType sort_order;