Wireshark · Ethereal-dev: [Ethereal-dev] [patch] small speed improvement

[didier <dgautheron@xxxxxxxx>]

Hi
reassemble.diff : for the common case when  packets are in order 
shortcut a loop. It matters a least for AFP where 200 frames read/write 
PDU aren't uncommon.
I'll try to do the same for LINK_FRAG.

packet.diff: move  if (pinfo->in_error_pkt) case in its own function, 
TRY allocates a lot of space on the stack and call_dissector_work is a 
recursive function.
Remove all but one volatile.

Anyone knows why there's so many volatile declarations? Other than too 
much copy and paste?

Didier

Index: epan/reassemble.c
===================================================================
--- epan/reassemble.c	(revision 13695)
+++ epan/reassemble.c	(working copy)
@@ -204,6 +204,21 @@
 	return TRUE;
 }
 
+/* ------------------------- */
+static fragment_data *new_head(guint32 flags)
+{
+	fragment_data *fd_head;
+	/* head/first structure in list only holds no other data than
+         * 'datalen' then we don't have to change the head of the list
+         * even if we want to keep it sorted
+         */
+	fd_head=g_mem_chunk_alloc(fragment_data_chunk);
+	memset(fd_head, 0, sizeof(fragment_data));
+        
+	fd_head->flags=flags;
+	return fd_head;
+}
+
 /*
  * For a reassembled-packet hash table entry, free the fragment data
  * to which the value refers.
@@ -580,6 +595,8 @@
 	fd->next = NULL;
 	fd->flags = 0;
 	fd->frame = pinfo->fd->num;
+	if (fd->frame > fd_head->frame)
+	    fd_head->frame = fd->frame;
 	fd->offset = frag_offset;
 	fd->len  = frag_data_len;
 	fd->data = NULL;
@@ -678,6 +695,7 @@
 
 	/* check if we have received the entire fragment
 	 * this is easy since the list is sorted and the head is faked.
+	 * common case the whole list is scanned.
 	 */
 	max = 0;
 	for (fd_i=fd_head->next;fd_i;fd_i=fd_i->next) {
@@ -785,9 +803,11 @@
 	 * to anything, and because it doesn't count in any case.
 	 */
 	if (!already_added && check_already_added && fd_head != NULL) {
-		for(fd_item=fd_head->next;fd_item;fd_item=fd_item->next){
-			if(pinfo->fd->num==fd_item->frame){
-				already_added=TRUE;
+		if (pinfo->fd->num <= fd_head->frame) {
+			for(fd_item=fd_head->next;fd_item;fd_item=fd_item->next){
+				if(pinfo->fd->num==fd_item->frame){
+					already_added=TRUE;
+				}
 			}
 		}
 	}
@@ -804,20 +824,8 @@
 		/* not found, this must be the first snooped fragment for this
                  * packet. Create list-head.
 		 */
-		fd_head=g_mem_chunk_alloc(fragment_data_chunk);
+		fd_head= new_head(0);
 
-		/* head/first structure in list only holds no other data than
-                 * 'datalen' then we don't have to change the head of the list
-                 * even if we want to keep it sorted
-                 */
-		fd_head->next=NULL;
-		fd_head->datalen=0;
-		fd_head->offset=0;
-		fd_head->len=0;
-		fd_head->flags=0;
-		fd_head->data=NULL;
-		fd_head->reassembled_in=0;
-
 		/*
 		 * We're going to use the key to insert the fragment,
 		 * so allocate a structure for it, and copy the
@@ -899,20 +907,8 @@
 		/* not found, this must be the first snooped fragment for this
                  * packet. Create list-head.
 		 */
-		fd_head=g_mem_chunk_alloc(fragment_data_chunk);
+		fd_head= new_head(0);
 
-		/* head/first structure in list only holds no other data than
-                 * 'datalen' then we don't have to change the head of the list
-                 * even if we want to keep it sorted
-                 */
-		fd_head->next=NULL;
-		fd_head->datalen=0;
-		fd_head->offset=0;
-		fd_head->len=0;
-		fd_head->flags=0;
-		fd_head->data=NULL;
-		fd_head->reassembled_in=0;
-
 		/*
 		 * We're going to use the key to insert the fragment,
 		 * so allocate a structure for it, and copy the
@@ -1248,20 +1244,8 @@
 		/* not found, this must be the first snooped fragment for this
                  * packet. Create list-head.
 		 */
-		fd_head=g_mem_chunk_alloc(fragment_data_chunk);
-
-		/* head/first structure in list only holds no other data than
-                 * 'datalen' then we don't have to change the head of the list
-                 * even if we want to keep it sorted
-                 */
-		fd_head->next=NULL;
-		fd_head->datalen=0;
-		fd_head->offset=0;
-		fd_head->len=0;
-		fd_head->flags=FD_BLOCKSEQUENCE;
-		fd_head->data=NULL;
-		fd_head->reassembled_in=0;
-
+		fd_head= new_head(FD_BLOCKSEQUENCE);
+		 
 		/*
 		 * We're going to use the key to insert the fragment,
 		 * so allocate a structure for it, and copy the
@@ -1320,20 +1304,8 @@
                /* not found, this must be the first snooped fragment for this
                  * packet. Create list-head.
                 */
-               fd_head=g_mem_chunk_alloc(fragment_data_chunk);
-
-               /* head/first structure in list only holds no other data than
-                 * 'datalen' then we don't have to change the head of the list
-                 * even if we want to keep it sorted
-                 */
-               fd_head->next=NULL;
-               fd_head->datalen=0;
-               fd_head->offset=0;
-               fd_head->len=0;
-               fd_head->flags=FD_BLOCKSEQUENCE;
-               fd_head->data=NULL;
-               fd_head->reassembled_in=0;
-
+	       fd_head= new_head(FD_BLOCKSEQUENCE);
+                
                /*
                 * We're going to use the key to insert the fragment,
                 * so allocate a structure for it, and copy the
@@ -1436,20 +1408,8 @@
 		/* not found, this must be the first snooped fragment for this
                  * packet. Create list-head.
 		 */
-		fd_head=g_mem_chunk_alloc(fragment_data_chunk);
+		fd_head= new_head(FD_BLOCKSEQUENCE);
 
-		/* head/first structure in list only holds no other data than
-                 * 'datalen' then we don't have to change the head of the list
-                 * even if we want to keep it sorted
-                 */
-		fd_head->next=NULL;
-		fd_head->datalen=0;
-		fd_head->offset=0;
-		fd_head->len=0;
-		fd_head->flags=FD_BLOCKSEQUENCE;
-		fd_head->data=NULL;
-		fd_head->reassembled_in=0;
-
 		if ((no_frag_number || frag_802_11_hack) && !more_frags) {
 			/*
 			 * This is the last fragment for this packet, and

Index: epan/packet.c
===================================================================
--- epan/packet.c	(revision 13695)
+++ epan/packet.c	(working copy)
@@ -393,21 +393,18 @@
  * the length of the tvbuff pointed to by the argument.
  */
 static int
+call_dissector_work_error(dissector_handle_t handle, tvbuff_t *tvb,
+    packet_info *pinfo, proto_tree *tree);
+
+static int
 call_dissector_work(dissector_handle_t handle, tvbuff_t *tvb,
     packet_info *pinfo_arg, proto_tree *tree)
 {
-	packet_info *volatile pinfo = pinfo_arg;
+	packet_info *pinfo = pinfo_arg;
 	const char *saved_proto;
 	guint16 saved_can_desegment;
-	volatile int ret;
-	gboolean save_writable;
-	volatile address save_dl_src;
-	volatile address save_dl_dst;
-	volatile address save_net_src;
-	volatile address save_net_dst;
-	volatile address save_src;
-	volatile address save_dst;
-	volatile gint saved_layer_names_len = 0;
+	int ret;
+	gint saved_layer_names_len = 0;
 
 	if (handle->protocol != NULL &&
 	    !proto_is_protocol_enabled(handle->protocol)) {
@@ -453,83 +450,84 @@
 	}
 
 	if (pinfo->in_error_pkt) {
+		ret = call_dissector_work_error(handle, tvb, pinfo, tree);
+	} else {
 		/*
-		 * This isn't a packet being transported inside
-		 * the protocol whose dissector is calling us,
-		 * it's a copy of a packet that caused an error
-		 * in some protocol included in a packet that
-		 * reports the error (e.g., an ICMP Unreachable
-		 * packet).
+		 * Just call the subdissector.
 		 */
+		ret = call_dissector_through_handle(handle, tvb, pinfo, tree);
+	}
 
+	if (ret == 0) {
 		/*
-		 * Save the current state of the writability of
-		 * the columns, and restore them after the
-		 * dissector returns, so that the columns
-		 * don't reflect the packet that got the error,
-		 * they reflect the packet that reported the
-		 * error.
+		 * That dissector didn't accept the packet, so
+		 * remove its protocol's name from the list
+		 * of protocols.
 		 */
-		save_writable = col_get_writable(pinfo->cinfo);
-		col_set_writable(pinfo->cinfo, FALSE);
-	 	save_dl_src = pinfo->dl_src;
-		save_dl_dst = pinfo->dl_dst;
-		save_net_src = pinfo->net_src;
-		save_net_dst = pinfo->net_dst;
-		save_src = pinfo->src;
-		save_dst = pinfo->dst;
-
-		/* Dissect the contained packet. */
-		TRY {
-			ret = call_dissector_through_handle(handle, tvb,
-			    pinfo, tree);
+		if (pinfo->layer_names != NULL) {
+			g_string_truncate(pinfo->layer_names,
+			    saved_layer_names_len);
 		}
-		CATCH(BoundsError) {
-			/*
-			 * Restore the column writability and addresses.
-			 */
-			col_set_writable(pinfo->cinfo, save_writable);
-			pinfo->dl_src = save_dl_src;
-			pinfo->dl_dst = save_dl_dst;
-			pinfo->net_src = save_net_src;
-			pinfo->net_dst = save_net_dst;
-			pinfo->src = save_src;
-			pinfo->dst = save_dst;
+	}
+	pinfo->current_proto = saved_proto;
+	pinfo->can_desegment = saved_can_desegment;
+	return ret;
+}
 
-			/*
-			 * Restore the current protocol, so any
-			 * "Short Frame" indication reflects that
-			 * protocol, not the protocol for the
-			 * packet that got the error.
-			 */
-			pinfo->current_proto = saved_proto;
+static int
+call_dissector_work_error(dissector_handle_t handle, tvbuff_t *tvb,
+    packet_info *pinfo_arg, proto_tree *tree)
+{
+	packet_info *pinfo = pinfo_arg;
+	const char *saved_proto;
+	guint16 saved_can_desegment;
+	volatile int ret;
+	gboolean save_writable;
+	address save_dl_src;
+	address save_dl_dst;
+	address save_net_src;
+	address save_net_dst;
+	address save_src;
+	address save_dst;
 
-			/*
-			 * Restore the desegmentability state.
-			 */
-			pinfo->can_desegment = saved_can_desegment;
+	saved_proto = pinfo->current_proto;
+	saved_can_desegment = pinfo->can_desegment;
 
-			/*
-			 * Rethrow the exception, so this will be
-			 * reported as a short frame.
-			 */
-			RETHROW;
-		}
-		CATCH(ReportedBoundsError) {
-			/*
-			 * "ret" wasn't set because an exception was thrown
-			 * before "call_dissector_through_handle()" returned.
-			 * As it called something, at least one dissector
-			 * accepted the packet, and, as an exception was
-			 * thrown, not only was all the tvbuff dissected,
-			 * a dissector tried dissecting past the end of
-			 * the data in some tvbuff, so we'll assume that
-			 * the entire tvbuff was dissected.
-			 */
-			ret = tvb_length(tvb);
-		}
-		ENDTRY;
+	/*
+	 * This isn't a packet being transported inside
+	 * the protocol whose dissector is calling us,
+	 * it's a copy of a packet that caused an error
+	 * in some protocol included in a packet that
+	 * reports the error (e.g., an ICMP Unreachable
+	 * packet).
+	 */
 
+	/*
+	 * Save the current state of the writability of
+	 * the columns, and restore them after the
+	 * dissector returns, so that the columns
+	 * don't reflect the packet that got the error,
+	 * they reflect the packet that reported the
+	 * error.
+	 */
+	save_writable = col_get_writable(pinfo->cinfo);
+	col_set_writable(pinfo->cinfo, FALSE);
+ 	save_dl_src = pinfo->dl_src;
+	save_dl_dst = pinfo->dl_dst;
+	save_net_src = pinfo->net_src;
+	save_net_dst = pinfo->net_dst;
+	save_src = pinfo->src;
+	save_dst = pinfo->dst;
+
+	/* Dissect the contained packet. */
+	TRY {
+		ret = call_dissector_through_handle(handle, tvb,
+			    pinfo, tree);
+	}
+	CATCH(BoundsError) {
+		/*
+		 * Restore the column writability and addresses.
+		 */
 		col_set_writable(pinfo->cinfo, save_writable);
 		pinfo->dl_src = save_dl_src;
 		pinfo->dl_dst = save_dl_dst;
@@ -537,27 +535,48 @@
 		pinfo->net_dst = save_net_dst;
 		pinfo->src = save_src;
 		pinfo->dst = save_dst;
-		pinfo->want_pdu_tracking = 0;
-	} else {
 		/*
-		 * Just call the subdissector.
+		 * Restore the current protocol, so any
+		 * "Short Frame" indication reflects that
+		 * protocol, not the protocol for the
+		 * packet that got the error.
 		 */
-		ret = call_dissector_through_handle(handle, tvb, pinfo, tree);
-	}
+		pinfo->current_proto = saved_proto;
 
-	if (ret == 0) {
 		/*
-		 * That dissector didn't accept the packet, so
-		 * remove its protocol's name from the list
-		 * of protocols.
+		 * Restore the desegmentability state.
 		 */
-		if (pinfo->layer_names != NULL) {
-			g_string_truncate(pinfo->layer_names,
-			    saved_layer_names_len);
-		}
+		pinfo->can_desegment = saved_can_desegment;
+
+		/*
+		 * Rethrow the exception, so this will be
+		 * reported as a short frame.
+		 */
+		RETHROW;
 	}
-	pinfo->current_proto = saved_proto;
-	pinfo->can_desegment = saved_can_desegment;
+	CATCH(ReportedBoundsError) {
+		/*
+		 * "ret" wasn't set because an exception was thrown
+		 * before "call_dissector_through_handle()" returned.
+		 * As it called something, at least one dissector
+		 * accepted the packet, and, as an exception was
+		 * thrown, not only was all the tvbuff dissected,
+		 * a dissector tried dissecting past the end of
+		 * the data in some tvbuff, so we'll assume that
+		 * the entire tvbuff was dissected.
+		 */
+		ret = tvb_length(tvb);
+	}
+	ENDTRY;
+
+	col_set_writable(pinfo->cinfo, save_writable);
+	pinfo->dl_src = save_dl_src;
+	pinfo->dl_dst = save_dl_dst;
+	pinfo->net_src = save_net_src;
+	pinfo->net_dst = save_net_dst;
+	pinfo->src = save_src;
+	pinfo->dst = save_dst;
+	pinfo->want_pdu_tracking = 0;
 	return ret;
 }