Wireshark · Ethereal-dev: Re: [ethereal-dev] Ethereal memory leakage fix and feature enhancement

[Guy Harris <gharris@xxxxxxxxxxxx>]

> > I'll see if there are any other ways to have GTK+ avoid looking for the
> > Nth element of the list when that's possible.
> 
> We also build a GList of frames by doing "g_list_append()" for each
> frame; building a list in that fashion is quadratic in the ultimate size
> of the list, as "g_list_append()" is linear in the current size of the
> list.
> 
> Adding a "next" pointer to "frame_data", and maintaining the list
> ourselves, rather than maintaining aa GList of pointers to "frame_data"
> structures, significantly sped up the tests I was doing reading in a
> large capture file.

...and keeping in a "frame_data" structure the GtkClist row number for
that frame (or -1 if it's not selected by the display filter), rather
than associating a pointer to the frame with the row, got rid of almost
all the CPU time spent looking for the Nth element of the list.

(I'm not sure how I managed to reduce "g_list_append()" CPU time to
almost nothing, given that the code to add a row to a GtkClist uses it;
I'm obviously missing something.

The two changes in question cut the CPU time spent reading in the
capture file in question from 36.12 seconds to 3.25 seconds - and the
loop that reads in that list appears to be CPU-bound (my disk makes
little noise, probably because the entire file fits in memory, and
remained cached); the wall-clock time in a test (somewhat crude, given
that

	1) there was some display updating going on

and

	2) the execution of Ethereal was terminated by me clicking on
	   the "close window" button, so some of the wall-clock time was
	   due to meatware)

went from 35 seconds to about 4 seconds.

I've attached a context diff of the changes; absent any objections, I'm
inclined to check them in.

Index: ethereal.c
===================================================================
RCS file: /usr/local/cvsroot/ethereal/ethereal.c,v
retrieving revision 1.77
diff -c -r1.77 ethereal.c
*** ethereal.c	1999/08/08 01:29:15	1.77
--- ethereal.c	1999/08/09 02:33:31
***************
*** 1012,1017 ****
--- 1012,1018 ----
      
    /* Initialize the capture file struct */
    cf.plist		= NULL;
+   cf.plist_end		= NULL;
    cf.wth		= NULL;
    cf.fh			= NULL;
    cf.dfilter		= NULL;

Index: file.c
===================================================================
RCS file: /usr/local/cvsroot/ethereal/file.c,v
retrieving revision 1.58
diff -c -r1.58 file.c
*** file.c	1999/08/08 01:29:14	1.58
--- file.c	1999/08/09 02:33:33
***************
*** 150,164 ****
    return (0);
  }
  
- static void
- free_packets_cb(gpointer data, gpointer user_data)
- {
-   g_free(data);
- }
- 
  /* Reset everything to a pristine state */
  void
  close_cap_file(capture_file *cf, void *w, guint context) {
    if (cf->fh) {
      fclose(cf->fh);
      cf->fh = NULL;
--- 150,160 ----
    return (0);
  }
  
  /* Reset everything to a pristine state */
  void
  close_cap_file(capture_file *cf, void *w, guint context) {
+   frame_data *fd, *fd_next;
+ 
    if (cf->fh) {
      fclose(cf->fh);
      cf->fh = NULL;
***************
*** 167,177 ****
      wtap_close(cf->wth);
      cf->wth = NULL;
    }
!   if (cf->plist) {
!     g_list_foreach(cf->plist, free_packets_cb, NULL);
!     g_list_free(cf->plist);
!     cf->plist = NULL;
    }
    unselect_packet(cf);	/* nothing to select */
  
    gtk_clist_freeze(GTK_CLIST(packet_list));
--- 163,174 ----
      wtap_close(cf->wth);
      cf->wth = NULL;
    }
!   for (fd = cf->plist; fd != NULL; fd = fd_next) {
!     fd_next = fd->next;
!     g_free(fd);
    }
+   cf->plist = NULL;
+   cf->plist_end = NULL;
    unselect_packet(cf);	/* nothing to select */
  
    gtk_clist_freeze(GTK_CLIST(packet_list));
***************
*** 451,464 ****
    }
    if (fdata->passed_dfilter) {
  	row = gtk_clist_append(GTK_CLIST(packet_list), fdata->cinfo->col_data);
! 	gtk_clist_set_row_data(GTK_CLIST(packet_list), row, fdata);
  
  	/* If this was the selected packet, remember the row it's in, so
  	   we can re-select it.  ("selected_packet" is 0-origin, as it's
  	   a GList index; "count", however, is 1-origin.) */
  	if (cf->selected_packet == cf->count - 1)
  	  cf->selected_row = row;
!   }
    fdata->cinfo = NULL;
  }
  
--- 448,462 ----
    }
    if (fdata->passed_dfilter) {
  	row = gtk_clist_append(GTK_CLIST(packet_list), fdata->cinfo->col_data);
! 	fdata->row = row;
  
  	/* If this was the selected packet, remember the row it's in, so
  	   we can re-select it.  ("selected_packet" is 0-origin, as it's
  	   a GList index; "count", however, is 1-origin.) */
  	if (cf->selected_packet == cf->count - 1)
  	  cf->selected_row = row;
!   } else
! 	fdata->row = -1;	/* not in the display */
    fdata->cinfo = NULL;
  }
  
***************
*** 467,474 ****
    const u_char *buf) {
    frame_data   *fdata;
    capture_file *cf = (capture_file *) user;
!   int passed;
    proto_tree   *protocol_tree;
  
    while (gtk_events_pending())
      gtk_main_iteration();
--- 465,473 ----
    const u_char *buf) {
    frame_data   *fdata;
    capture_file *cf = (capture_file *) user;
!   int           passed;
    proto_tree   *protocol_tree;
+   frame_data   *plist_end;
  
    while (gtk_events_pending())
      gtk_main_iteration();
***************
*** 493,527 ****
      proto_tree_free(protocol_tree);
    }
    if (passed) {
!     cf->plist = g_list_append(cf->plist, (gpointer) fdata);
  
      cf->count++;
      add_packet_to_packet_list(fdata, cf, buf);
!   } else g_free(fdata);
  }
  
- static void
- filter_packets_cb(gpointer data, gpointer user_data)
- {
-   frame_data *fd = data;
-   capture_file *cf = user_data;
- 
-   cf->count++;
- 
-   fseek(cf->fh, fd->file_off, SEEK_SET);
-   fread(cf->pd, sizeof(guint8), fd->cap_len, cf->fh);
- 
-   add_packet_to_packet_list(fd, cf, cf->pd);
- 
-   if (cf->count % 20 == 0) {
- 	  dfilter_progress_cb(cf);
-   }
- }
- 
  void
  filter_packets(capture_file *cf)
  {
  /*  gint timeout;*/
  
    if (cf->dfilter != NULL) {
      /*
--- 492,515 ----
      proto_tree_free(protocol_tree);
    }
    if (passed) {
!     plist_end = cf->plist_end;
!     if (plist_end != NULL)
!       plist_end->next = fdata;
!     else
!       cf->plist = fdata;
!     cf->plist_end = fdata;
  
      cf->count++;
      add_packet_to_packet_list(fdata, cf, buf);
!   } else
!     g_free(fdata);
  }
  
  void
  filter_packets(capture_file *cf)
  {
  /*  gint timeout;*/
+   frame_data *fd;
  
    if (cf->dfilter != NULL) {
      /*
***************
*** 558,564 ****
    cf->count = 0;
  
  /*  timeout = gtk_timeout_add(250, dfilter_progress_cb, cf);*/
!   g_list_foreach(cf->plist, filter_packets_cb, cf);
  /*  gtk_timeout_remove(timeout);*/
   
    gtk_progress_bar_update(GTK_PROGRESS_BAR(prog_bar), 0);
--- 546,563 ----
    cf->count = 0;
  
  /*  timeout = gtk_timeout_add(250, dfilter_progress_cb, cf);*/
!   for (fd = cf->plist; fd != NULL; fd = fd->next) {
!     cf->count++;
! 
!     fseek(cf->fh, fd->file_off, SEEK_SET);
!     fread(cf->pd, sizeof(guint8), fd->cap_len, cf->fh);
! 
!     add_packet_to_packet_list(fd, cf, cf->pd);
! 
!     if (cf->count % 20 == 0) {
!       dfilter_progress_cb(cf);
!     }
!   }
  /*  gtk_timeout_remove(timeout);*/
   
    gtk_progress_bar_update(GTK_PROGRESS_BAR(prog_bar), 0);
***************
*** 596,626 ****
  }
  
  
- static void
- print_packets_cb(gpointer data, gpointer user_data)
- {
-   frame_data *fd = data;
-   capture_file *cf = user_data;
-   proto_tree *protocol_tree;
- 
-   cf->count++;
- 
-   fseek(cf->fh, fd->file_off, SEEK_SET);
-   fread(cf->pd, sizeof(guint8), fd->cap_len, cf->fh);
- 
-   /* create the logical protocol tree */
-   protocol_tree = proto_tree_create_root();
-   dissect_packet(cf->pd, fd, protocol_tree);
- 
-   /* Print the packet */
-   proto_tree_print(cf->count, (GNode *)protocol_tree, cf->pd, fd, cf->print_fh);
- 
-   proto_tree_free(protocol_tree);
- }
- 
  int
  print_packets(capture_file *cf, int to_file, const char *dest)
  {
    cf->print_fh = open_print_dest(to_file, dest);
    if (cf->print_fh == NULL)
      return FALSE;	/* attempt to open destination failed */
--- 595,606 ----
  }
  
  
  int
  print_packets(capture_file *cf, int to_file, const char *dest)
  {
+   frame_data *fd;
+   proto_tree *protocol_tree;
+ 
    cf->print_fh = open_print_dest(to_file, dest);
    if (cf->print_fh == NULL)
      return FALSE;	/* attempt to open destination failed */
***************
*** 638,644 ****
     * Iterate through the list of packets, printing each of them.
     */
    cf->count = 0;
!   g_list_foreach(cf->plist, print_packets_cb, cf);
  
  #if 0
    print_finale(cf->print_fh);
--- 618,638 ----
     * Iterate through the list of packets, printing each of them.
     */
    cf->count = 0;
!   for (fd = cf->plist; fd != NULL; fd = fd->next) {
!     cf->count++;
! 
!     fseek(cf->fh, fd->file_off, SEEK_SET);
!     fread(cf->pd, sizeof(guint8), fd->cap_len, cf->fh);
! 
!     /* create the logical protocol tree */
!     protocol_tree = proto_tree_create_root();
!     dissect_packet(cf->pd, fd, protocol_tree);
! 
!     /* Print the packet */
!     proto_tree_print(cf->count, (GNode *)protocol_tree, cf->pd, fd, cf->print_fh);
! 
!     proto_tree_free(protocol_tree);
!   }
  
  #if 0
    print_finale(cf->print_fh);
***************
*** 649,697 ****
    return TRUE;
  }
  
- static void
- change_time_formats_cb(gpointer data, gpointer user_data)
- {
-   frame_data *fd = data;
-   capture_file *cf = user_data;
-   gint          i;
- 
-   cf->count++;
- 
-   /* XXX - there really should be a way of checking "cf->cinfo" for this;
-      the answer isn't going to change from packet to packet, so we should
-      simply skip all the "change_time_formats()" work if we're not
-      changing anything. */
-   fd->cinfo = &cf->cinfo;
-   if (!check_col(fd, COL_CLS_TIME)) {
-     /* There are no columns that show the time in the "command-line-specified"
-        format, so there's nothing we need to do. */
-     return;
-   }
- 
-   compute_time_stamps(fd, cf);
- 
-   for (i = 0; i < fd->cinfo->num_cols; i++) {
-     fd->cinfo->col_data[i][0] = '\0';
-   }
-   col_add_cls_time(fd);
-   for (i = 0; i < fd->cinfo->num_cols; i++) {
-     if (fd->cinfo->fmt_matx[i][COL_CLS_TIME]) {
-       /* This is one of the columns that shows the time in
-          "command-line-specified" format; update it. */
-       gtk_clist_set_text(GTK_CLIST(packet_list), cf->count - 1, i,
- 			  fd->cinfo->col_data[i]);
-     }
-   }
-   fd->cinfo = NULL;
- }
- 
  /* Scan through the packet list and change all columns that use the
     "command-line-specified" time stamp format to use the current
     value of that format. */
  void
  change_time_formats(capture_file *cf)
  {
    int i;
    GtkStyle  *pl_style;
  
--- 643,655 ----
    return TRUE;
  }
  
  /* Scan through the packet list and change all columns that use the
     "command-line-specified" time stamp format to use the current
     value of that format. */
  void
  change_time_formats(capture_file *cf)
  {
+   frame_data *fd;
    int i;
    GtkStyle  *pl_style;
  
***************
*** 709,715 ****
    lastsec = 0;
    lastusec = 0;
    cf->count = 0;
!   g_list_foreach(cf->plist, change_time_formats_cb, cf);
  
    /* Set the column widths of those columns that show the time in
       "command-line-specified" format. */
--- 667,698 ----
    lastsec = 0;
    lastusec = 0;
    cf->count = 0;
!   for (fd = cf->plist; fd != NULL; fd = fd->next) {
!     cf->count++;
! 
!     /* XXX - there really should be a way of checking "cf->cinfo" for this;
!        the answer isn't going to change from packet to packet, so we should
!        simply skip all the "change_time_formats()" work if we're not
!        changing anything. */
!     if (check_col(fd, COL_CLS_TIME)) {
!       /* There are columns that show the time in the "command-line-specified"
!          format; update them. */
!       compute_time_stamps(fd, cf);
! 
!       for (i = 0; i < cf->cinfo.num_cols; i++) {
!         cf->cinfo.col_data[i][0] = '\0';
!       }
!       col_add_cls_time(fd);
!       for (i = 0; i < cf->cinfo.num_cols; i++) {
!         if (cf->cinfo.fmt_matx[i][COL_CLS_TIME]) {
!           /* This is one of the columns that shows the time in
!              "command-line-specified" format; update it. */
!           gtk_clist_set_text(GTK_CLIST(packet_list), cf->count - 1, i,
! 			  cf->cinfo.col_data[i]);
!         }
!       }
!     }
!   }
  
    /* Set the column widths of those columns that show the time in
       "command-line-specified" format. */
***************
*** 744,764 ****
  void
  select_packet(capture_file *cf, int row)
  {
    /* Clear out whatever's currently in the hex dump. */
    gtk_text_freeze(GTK_TEXT(byte_view));
    gtk_text_set_point(GTK_TEXT(byte_view), 0);
    gtk_text_forward_delete(GTK_TEXT(byte_view),
      gtk_text_get_length(GTK_TEXT(byte_view)));
  
!   /* Get the frame data struct pointer for this frame. */
!   cf->fd = (frame_data *) gtk_clist_get_row_data(GTK_CLIST(packet_list), row);
  
    /* Get the data in that frame. */
    fseek(cf->fh, cf->fd->file_off, SEEK_SET);
    fread(cf->pd, sizeof(guint8), cf->fd->cap_len, cf->fh);
- 
-   /* Mark that frame as the selected frame. */
-   cf->selected_packet = g_list_index(cf->plist, (gpointer)cf->fd);
  
    /* Create the logical protocol tree. */
    if (cf->protocol_tree)
--- 727,755 ----
  void
  select_packet(capture_file *cf, int row)
  {
+   frame_data *fd;
+   int i;
+ 
    /* Clear out whatever's currently in the hex dump. */
    gtk_text_freeze(GTK_TEXT(byte_view));
    gtk_text_set_point(GTK_TEXT(byte_view), 0);
    gtk_text_forward_delete(GTK_TEXT(byte_view),
      gtk_text_get_length(GTK_TEXT(byte_view)));
  
!   /* Search through the list of frames to see which one is in
!      this row. */
!   for (fd = cf->plist, i = 0; fd != NULL; fd = fd->next, i++) {
!     if (fd->row == row)
!       break;
!   }
!   cf->fd = fd;
  
+   /* Remember the ordinal number of that frame. */
+   cf->selected_packet = i;
+ 
    /* Get the data in that frame. */
    fseek(cf->fh, cf->fd->file_off, SEEK_SET);
    fread(cf->pd, sizeof(guint8), cf->fd->cap_len, cf->fh);
  
    /* Create the logical protocol tree. */
    if (cf->protocol_tree)

Index: file.h
===================================================================
RCS file: /usr/local/cvsroot/ethereal/file.h,v
retrieving revision 1.26
diff -c -r1.26 file.h
*** file.h	1999/08/07 17:28:21	1.26
--- file.h	1999/08/09 02:33:34
***************
*** 76,82 ****
     */
    /*guint8      pd[MAX_PACKET_SIZE];*/  /* Packet data */
    guint8      pd[65536];  /* Packet data */
!   GList      *plist;     /* Packet list */
    column_info  cinfo;    /* Column formatting information */
    int        selected_packet;   /* Index in packet list of currently selected packet, if any */
    int        selected_row;   /* Row in packet display of currently selected packet, if any */
--- 76,83 ----
     */
    /*guint8      pd[MAX_PACKET_SIZE];*/  /* Packet data */
    guint8      pd[65536];  /* Packet data */
!   frame_data *plist;     /* Packet list */
!   frame_data *plist_end; /* Last packet in list */
    column_info  cinfo;    /* Column formatting information */
    int        selected_packet;   /* Index in packet list of currently selected packet, if any */
    int        selected_row;   /* Row in packet display of currently selected packet, if any */

Index: packet.h
===================================================================
RCS file: /usr/local/cvsroot/ethereal/packet.h,v
retrieving revision 1.79
diff -c -r1.79 packet.h
*** packet.h	1999/08/04 04:37:45	1.79
--- packet.h	1999/08/09 02:33:35
***************
*** 102,107 ****
--- 102,108 ----
  } packet_counts;
  
  typedef struct _frame_data {
+   struct _frame_data *next; /* Next element in list */
    guint32      pkt_len;   /* Packet length */
    guint32      cap_len;   /* Amount actually captured */
    guint32      rel_secs;  /* Relative seconds */
***************
*** 112,117 ****
--- 113,119 ----
    guint32      del_usecs; /* Delta microseconds */
    long         file_off;  /* File offset */
    column_info *cinfo;     /* Column formatting information */
+   gint         row;       /* Row number for this packet in the display */
    int          lnk_t;     /* Per-packet encapsulation/data-link type */
    gboolean     passed_dfilter; /* TRUE = display, FALSE = no display */
    guint8       flags;     /* for ENCAP_LAPB : 1st bit means From DCE */

Index: summary.c
===================================================================
RCS file: /usr/local/cvsroot/ethereal/summary.c,v
retrieving revision 1.7
diff -c -r1.7 summary.c
*** summary.c	1999/08/02 02:04:27	1.7
--- summary.c	1999/08/09 02:33:35
***************
*** 128,137 ****
  
   guint32        traffic_bytes, i;
   double         seconds;
!  GList          *cur_glist;
  
   /* initialize the tally */
!   first_frame = (frame_data *)(cf.plist->data);
    st = (summary_tally *)g_malloc(sizeof(summary_tally));
    st->start_time = secs_usecs(first_frame->abs_secs,first_frame->abs_usecs) 
  ;
--- 128,137 ----
  
   guint32        traffic_bytes, i;
   double         seconds;
!  frame_data    *cur_glist;
  
   /* initialize the tally */
!   first_frame = cf.plist;
    st = (summary_tally *)g_malloc(sizeof(summary_tally));
    st->start_time = secs_usecs(first_frame->abs_secs,first_frame->abs_usecs) 
  ;
***************
*** 142,153 ****
    cur_glist = cf.plist;
  
    for (i = 0; i < cf.count; i++){
!     cur_frame = (frame_data *)cur_glist->data;
      tally_frame_data(cur_frame, st);
      cur_glist = cur_glist->next;
!     }
! 
!   /*  g_list_foreach(cf.plist_first, (GFunc)tally_frame_data, st); */
  
    /* traffic_bytes will be computed here */
    traffic_bytes = st->bytes;
--- 142,151 ----
    cur_glist = cf.plist;
  
    for (i = 0; i < cf.count; i++){
!     cur_frame = cur_glist;
      tally_frame_data(cur_frame, st);
      cur_glist = cur_glist->next;
!   }
  
    /* traffic_bytes will be computed here */
    traffic_bytes = st->bytes;