[GTK] Speed up Combo setItems/removeAll/remove for large item counts#3401
Draft
vogella wants to merge 1 commit into
Draft
[GTK] Speed up Combo setItems/removeAll/remove for large item counts#3401vogella wants to merge 1 commit into
vogella wants to merge 1 commit into
Conversation
Contributor
Setting or removing a large number of combo items (>5000) was very slow on GTK because GtkComboBox recomputes its popup/cell-view layout on every single model change, leading to O(n^2)/O(n) per-row overhead. Detach the GtkListStore model from the combo box before bulk modifying it (setItems, removeAll and remove(start, end)) and re-attach it afterwards, so the widget reacts only once. setItems now populates the GtkListStore directly instead of going through the per-item GtkComboBoxText convenience function. remove(start, end) restores the active selection after re-attaching, adjusted for the rows removed before it. Adds the gtk_combo_box_set_model native binding and drops the now-unused gtk_combo_box_text_remove_all binding. Fixes eclipse-platform#506
vogella
force-pushed
the
gtk-combo-bulk-update-perf
branch
from
53c2e10 to
924ec33
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves GTK Combo performance for large bulk updates by avoiding repeated GtkComboBox relayouts on every single row change. It does so by temporarily detaching the GtkListStore model during setItems, removeAll, and range remove(start, end), then reattaching it once, and it adds a GTK-only unit test to validate selection preservation behavior for range removals.
Changes:
- Detach/reattach the
GtkComboBoxmodel during bulkCombomutations to reduce relayout work on GTK. - Preserve and restore the active selection across
remove(start, end)when the selected item is outside the removed range. - Add a new GTK native binding (
gtk_combo_box_set_model) and a GTK-only JUnit test for selection handling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Combo.java | Adds a GTK-only test to validate selection behavior around range removals. |
| bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Combo.java | Implements model detachment for bulk operations and restores selection for range removes. |
| bundles/org.eclipse.swt/Eclipse SWT PI/gtk/org/eclipse/swt/internal/gtk/GTK.java | Adds gtk_combo_box_set_model binding and removes gtk_combo_box_text_remove_all binding. |
| bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os.c | Adds JNI bridge for gtk_combo_box_set_model and removes JNI bridge for gtk_combo_box_text_remove_all. |
| bundles/org.eclipse.swt/Eclipse SWT PI/gtk/library/os_stats.h | Updates native function stats enum for the added/removed GTK JNI bindings. |
Comment on lines
+2080
to
+2098
| OS.g_object_ref (model); | ||
| gtk_combo_box_toggle_wrap (false); | ||
| GTK.gtk_combo_box_set_model (handle, 0); | ||
| long iter = OS.g_malloc (GTK.GtkTreeIter_sizeof ()); | ||
| for (int i = end; i >= start; i--) { | ||
| if (GTK.gtk_tree_model_iter_nth_child (model, iter, 0, i)) { | ||
| GTK.gtk_list_store_remove (model, iter); | ||
| } | ||
| } | ||
| OS.g_free (iter); | ||
| GTK.gtk_combo_box_set_model (handle, model); | ||
| if (newIndex != -1) { | ||
| // Restore the selection without firing a spurious Modify event. | ||
| OS.g_signal_handlers_block_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED); | ||
| GTK.gtk_combo_box_set_active (handle, newIndex); | ||
| OS.g_signal_handlers_unblock_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED); | ||
| } | ||
| OS.g_object_unref (model); | ||
| gtk_combo_box_toggle_wrap (true); |
Comment on lines
+2144
to
+2150
| OS.g_object_ref (model); | ||
| gtk_combo_box_toggle_wrap (false); | ||
| GTK.gtk_combo_box_set_model (handle, 0); | ||
| GTK.gtk_list_store_clear (model); | ||
| GTK.gtk_combo_box_set_model (handle, model); | ||
| OS.g_object_unref (model); | ||
| gtk_combo_box_toggle_wrap (true); |
Comment on lines
+2444
to
+2455
| OS.g_object_ref (model); | ||
| gtk_combo_box_toggle_wrap (false); | ||
| GTK.gtk_combo_box_set_model (handle, 0); | ||
| GTK.gtk_list_store_clear (model); | ||
| long iter = OS.g_malloc (GTK.GtkTreeIter_sizeof ()); | ||
| for (int i = 0; i < items.length; i++) { | ||
| gtk_list_store_insert (model, iter, items [i], i); | ||
| } | ||
| OS.g_free (iter); | ||
| GTK.gtk_combo_box_set_model (handle, model); | ||
| OS.g_object_unref (model); | ||
| gtk_combo_box_toggle_wrap (true); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Setting or clearing a large number of
Comboitems (>5000) is very slow on GTK becauseGtkComboBoxrecomputes its popup and cell-view layout on every single model change.This detaches the
GtkListStoreduring the bulksetItems,removeAllandremove(start, end)operations and re-attaches it once, so the widget relayouts a single time instead of once per row.The active selection is preserved across range removals, and a new
Combotest covers that selection handling.This adds one native binding (
gtk_combo_box_set_model), so it needs the GTK3 and GTK4 CI runs to validate the native build and behavior; it could not be compiled or tested locally.Fixes #506