From e7d5c49fe1dadfd6393539ba1b4d535087d79abe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mo=C5=84?= Date: Sat, 28 Jan 2023 13:41:50 +0100 Subject: [PATCH] epan: Use hash table for dependent frames Dependent frames list order does not matter and thus significantly faster data structure can be used. Replace the list with hash table to avoid excessive CPU usage when opening files containing reassembled packets consisting of large number of fragments. --- epan/frame_data.c | 4 ++-- epan/frame_data.h | 2 +- epan/frame_data_sequence.c | 8 +++++--- epan/frame_data_sequence.h | 2 +- epan/packet.c | 9 +++------ file.c | 4 ++-- packaging/debian/libwireshark0.symbols | 2 +- sharkd.c | 4 ++-- tfshark.c | 4 ++-- tshark.c | 4 ++-- 10 files changed, 21 insertions(+), 22 deletions(-) diff --git a/epan/frame_data.c b/epan/frame_data.c index 59673d0f0d..fb8f736dcb 100644 --- a/epan/frame_data.c +++ b/epan/frame_data.c @@ -300,7 +300,7 @@ frame_data_reset(frame_data *fdata) } if (fdata->dependent_frames) { - g_slist_free(fdata->dependent_frames); + g_hash_table_destroy(fdata->dependent_frames); fdata->dependent_frames = NULL; } } @@ -314,7 +314,7 @@ frame_data_destroy(frame_data *fdata) } if (fdata->dependent_frames) { - g_slist_free(fdata->dependent_frames); + g_hash_table_destroy(fdata->dependent_frames); fdata->dependent_frames = NULL; } } diff --git a/epan/frame_data.h b/epan/frame_data.h index b631cac2ac..acca7e772e 100644 --- a/epan/frame_data.h +++ b/epan/frame_data.h @@ -71,7 +71,7 @@ typedef struct _frame_data { LLP64 (64-bit Windows) platforms. Put them here, one after the other, so they don't require padding between them. */ GSList *pfd; /**< Per frame proto data */ - GSList *dependent_frames; /**< A list of frames which this one depends on */ + GHashTable *dependent_frames; /**< A hash table of frames which this one depends on */ const struct _color_filter *color_filter; /**< Per-packet matching color_filter_t object */ guint16 subnum; /**< subframe number, for protocols that require this */ /* Keep the bitfields below to 16 bits, so this plus the previous field diff --git a/epan/frame_data_sequence.c b/epan/frame_data_sequence.c index 144d6c1fcb..9ca039a49f 100644 --- a/epan/frame_data_sequence.c +++ b/epan/frame_data_sequence.c @@ -312,10 +312,10 @@ free_frame_data_sequence(frame_data_sequence *fds) } void -find_and_mark_frame_depended_upon(gpointer data, gpointer user_data) +find_and_mark_frame_depended_upon(gpointer key, gpointer value _U_, gpointer user_data) { frame_data *dependent_fd; - guint32 dependent_frame = GPOINTER_TO_UINT(data); + guint32 dependent_frame = GPOINTER_TO_UINT(key); frame_data_sequence *frames = (frame_data_sequence *)user_data; if (dependent_frame && frames) { @@ -325,7 +325,9 @@ find_and_mark_frame_depended_upon(gpointer data, gpointer user_data) */ if (!(dependent_fd->dependent_of_displayed || dependent_fd->passed_dfilter)) { dependent_fd->dependent_of_displayed = 1; - g_slist_foreach(dependent_fd->dependent_frames, find_and_mark_frame_depended_upon, frames); + if (dependent_fd->dependent_frames) { + g_hash_table_foreach(dependent_fd->dependent_frames, find_and_mark_frame_depended_upon, frames); + } } } } diff --git a/epan/frame_data_sequence.h b/epan/frame_data_sequence.h index 035609307d..aa11682971 100644 --- a/epan/frame_data_sequence.h +++ b/epan/frame_data_sequence.h @@ -33,7 +33,7 @@ WS_DLL_PUBLIC frame_data *frame_data_sequence_find(frame_data_sequence *fds, */ WS_DLL_PUBLIC void free_frame_data_sequence(frame_data_sequence *fds); -WS_DLL_PUBLIC void find_and_mark_frame_depended_upon(gpointer data, gpointer user_data); +WS_DLL_PUBLIC void find_and_mark_frame_depended_upon(gpointer key, gpointer value, gpointer user_data); #ifdef __cplusplus diff --git a/epan/packet.c b/epan/packet.c index 204e17696d..e69af8857e 100644 --- a/epan/packet.c +++ b/epan/packet.c @@ -452,13 +452,10 @@ mark_frame_as_depended_upon(frame_data *fd, guint32 frame_num) /* ws_assert(frame_num < fd->num) - we assume in several other * places in the code that frames don't depend on future * frames. */ - /* XXX: Looking to see if the frame is already there is slow - * if there's a lot of dependent frames, so this should - * be a hash table or something. - */ - if (g_slist_find(fd->dependent_frames, GUINT_TO_POINTER(frame_num)) == NULL) { - fd->dependent_frames = g_slist_prepend(fd->dependent_frames, GUINT_TO_POINTER(frame_num)); + if (fd->dependent_frames == NULL) { + fd->dependent_frames = g_hash_table_new(g_direct_hash, g_direct_equal); } + g_hash_table_insert(fd->dependent_frames, GUINT_TO_POINTER(frame_num), NULL); } } diff --git a/file.c b/file.c index a164626c8d..24bb3ba71b 100644 --- a/file.c +++ b/file.c @@ -1194,12 +1194,12 @@ add_packet_to_packet_list(frame_data *fdata, capture_file *cf, if (dfcode != NULL) { fdata->passed_dfilter = dfilter_apply_edt(dfcode, edt) ? 1 : 0; - if (fdata->passed_dfilter) { + if (fdata->passed_dfilter && edt->pi.fd->dependent_frames) { /* This frame passed the display filter but it may depend on other * (potentially not displayed) frames. Find those frames and mark them * as depended upon. */ - g_slist_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); + g_hash_table_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); } } else fdata->passed_dfilter = 1; diff --git a/packaging/debian/libwireshark0.symbols b/packaging/debian/libwireshark0.symbols index ae055fea74..db31c9ec3e 100644 --- a/packaging/debian/libwireshark0.symbols +++ b/packaging/debian/libwireshark0.symbols @@ -678,7 +678,7 @@ libwireshark.so.0 libwireshark0 #MINVER# fetch_tapped_data@Base 1.9.1 filter_expression_iterate_expressions@Base 2.5.0 filter_expression_new@Base 1.9.1 - find_and_mark_frame_depended_upon@Base 1.12.0~rc1 + find_and_mark_frame_depended_upon@Base 4.1.0 find_capture_dissector@Base 2.3.0 find_conversation@Base 1.9.1 find_conversation_by_id@Base 2.5.0 diff --git a/sharkd.c b/sharkd.c index 2dbc1cad86..d3602350f1 100644 --- a/sharkd.c +++ b/sharkd.c @@ -315,8 +315,8 @@ process_packet(capture_file *cf, epan_dissect_t *edt, * if a display filter was given and it matches this packet. */ if (edt && cf->dfcode) { - if (dfilter_apply_edt(cf->dfcode, edt)) { - g_slist_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); + if (dfilter_apply_edt(cf->dfcode, edt) && edt->pi.fd->dependent_frames) { + g_hash_table_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); } } diff --git a/tfshark.c b/tfshark.c index fd04fe42a0..35917a19cf 100644 --- a/tfshark.c +++ b/tfshark.c @@ -1071,8 +1071,8 @@ process_packet_first_pass(capture_file *cf, epan_dissect_t *edt, * More importantly, edt.pi.fd.dependent_frames won't be initialized because * epan hasn't been initialized. */ - if (edt) { - g_slist_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); + if (edt && edt->pi.fd->dependent_frames) { + g_hash_table_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); } cf->count++; diff --git a/tshark.c b/tshark.c index bbba6a97ba..bb457c6785 100644 --- a/tshark.c +++ b/tshark.c @@ -3056,8 +3056,8 @@ process_packet_first_pass(capture_file *cf, epan_dissect_t *edt, * if a display filter was given and it matches this packet. */ if (edt && cf->dfcode) { - if (dfilter_apply_edt(cf->dfcode, edt)) { - g_slist_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); + if (dfilter_apply_edt(cf->dfcode, edt) && edt->pi.fd->dependent_frames) { + g_hash_table_foreach(edt->pi.fd->dependent_frames, find_and_mark_frame_depended_upon, cf->provider.frames); } if (selected_frame_number != 0 && selected_frame_number == cf->count + 1) {