diff options
author | Gerald Combs <gerald@wireshark.org> | 2015-09-10 23:06:12 +0000 |
---|---|---|
committer | Gerald Combs <gerald@wireshark.org> | 2015-09-11 17:34:53 +0000 |
commit | 2931dc118b6c9090e03480811c38ca582f20f8f3 (patch) | |
tree | bbb2f70b0405a08fd39bfb6920eb2d166d0685fa | |
parent | c088135d5ba1846e2a7c20d6121c4352a83e68b9 (diff) |
Try using GStringChunks in PacketListRecord.
This saves a fair amount of memory in tests here. Loading a large
capture file and sorting on a custom column (tcp.window_size) uses 676
MB before the change and 634 after.
Add notes about possble further improvements:
Roll our own replacement for GStringChunks using wmem_tree.
Have PacketListRecord::columnString return a const char * instead of a
const QByteArray.
Change-Id: Icb36194f5ad290828d7106ccc3bf494d07d76d08
Reviewed-on: https://code.wireshark.org/review/10476
Reviewed-by: Gerald Combs <gerald@wireshark.org>
-rw-r--r-- | ui/qt/packet_list_model.cpp | 2 | ||||
-rw-r--r-- | ui/qt/packet_list_record.cpp | 32 | ||||
-rw-r--r-- | ui/qt/packet_list_record.h | 8 |
3 files changed, 32 insertions, 10 deletions
diff --git a/ui/qt/packet_list_model.cpp b/ui/qt/packet_list_model.cpp index 699ffe5c6e..5b10900158 100644 --- a/ui/qt/packet_list_model.cpp +++ b/ui/qt/packet_list_model.cpp @@ -51,6 +51,7 @@ PacketListModel::PacketListModel(QObject *parent, capture_file *cf) : line_spacing_(0) { setCaptureFile(cf); + PacketListRecord::clearStringPool(); connect(this, SIGNAL(itemHeightChanged(QModelIndex)), this, SLOT(emitItemHeightChanged(QModelIndex)), Qt::QueuedConnection); @@ -114,6 +115,7 @@ void PacketListModel::clear() { physical_rows_.clear(); visible_rows_.clear(); number_to_row_.clear(); + PacketListRecord::clearStringPool(); endResetModel(); } diff --git a/ui/qt/packet_list_record.cpp b/ui/qt/packet_list_record.cpp index cdf61bec12..73b788405d 100644 --- a/ui/qt/packet_list_record.cpp +++ b/ui/qt/packet_list_record.cpp @@ -47,6 +47,8 @@ PacketListRecord::PacketListRecord(frame_data *frameData) : { } +// We might want to return a const char * instead. This would keep us from +// creating excessive QByteArrays, e.g. in PacketListModel::recordLessThan. const QByteArray PacketListRecord::columnString(capture_file *cap_file, int column, bool colorized) { // packet_list_store.c:packet_list_get_value @@ -57,7 +59,7 @@ const QByteArray PacketListRecord::columnString(capture_file *cap_file, int colu } bool dissect_color = colorized && !colorized_; - if (column >= col_text_.size() || col_text_[column].isNull() || data_ver_ != col_data_ver_ || dissect_color) { + if (column >= col_text_.size() || !col_text_[column] || data_ver_ != col_data_ver_ || dissect_color) { dissect(cap_file, dissect_color); } @@ -172,6 +174,14 @@ void PacketListRecord::dissect(capture_file *cap_file, bool dissect_color) ws_buffer_free(&buf); } +// This assumes only one packet list. We might want to move this to +// PacketListModel (or replace this with a wmem allocator). +struct _GStringChunk *PacketListRecord::string_pool_ = g_string_chunk_new(1 * 1024 * 1024); +void PacketListRecord::clearStringPool() +{ + g_string_chunk_clear(string_pool_); +} + //#define MINIMIZE_STRING_COPYING 1 void PacketListRecord::cacheColumnStrings(column_info *cinfo) { @@ -244,23 +254,27 @@ void PacketListRecord::cacheColumnStrings(column_info *cinfo) break; } #else // MINIMIZE_STRING_COPYING - // XXX The GTK+ code uses GStringChunk for string storage. It - // doesn't appear to be that much faster, but it probably uses - // less memory. - QByteArray col_text; + const char *col_str; if (!get_column_resolved(column) && cinfo->col_expr.col_expr_val[column]) { /* Use the unresolved value in col_expr_val */ - col_text = cinfo->col_expr.col_expr_val[column]; + col_str = cinfo->col_expr.col_expr_val[column]; } else { int text_col = cinfo_column_.value(column, -1); if (text_col < 0) { col_fill_in_frame_data(fdata_, cinfo, column, FALSE); } - col_text = cinfo->columns[column].col_data; + col_str = cinfo->columns[column].col_data; + } + // g_string_chunk_insert_const manages a hash table of pointers to + // strings: + // https://git.gnome.org/browse/glib/tree/glib/gstringchunk.c + // We might be better off adding the equivalent functionality to + // wmem_tree. + col_text_.append(g_string_chunk_insert_const(string_pool_, col_str)); + for (int i = 0; col_str[i]; i++) { + if (col_str[i] == '\n') col_lines++; } - col_text_.append(col_text); - col_lines += col_text.count('\n'); if (col_lines > lines_) { lines_ = col_lines; line_count_changed_ = true; diff --git a/ui/qt/packet_list_record.h b/ui/qt/packet_list_record.h index c8d2e90a28..7c23c43ea8 100644 --- a/ui/qt/packet_list_record.h +++ b/ui/qt/packet_list_record.h @@ -36,6 +36,7 @@ #include <QVariant> struct conversation; +struct _GStringChunk; class PacketListRecord { @@ -55,9 +56,11 @@ public: inline int lineCount() { return lines_; } inline int lineCountChanged() { return line_count_changed_; } + static void clearStringPool(); + private: /** The column text for some columns */ - QList<QByteArray> col_text_; + QList<const char *> col_text_; frame_data *fdata_; int lines_; @@ -75,6 +78,9 @@ private: void dissect(capture_file *cap_file, bool dissect_color = false); void cacheColumnStrings(column_info *cinfo); + + static struct _GStringChunk *string_pool_; + }; #endif // PACKET_LIST_RECORD_H |