[GTK] Speed up Combo setItems/removeAll/remove for large item counts - #3401
[GTK] Speed up Combo setItems/removeAll/remove for large item counts#3401vogella wants to merge 1 commit into
Conversation
Test Results 211 files - 1 211 suites - 1 28m 0s ⏱️ +14s For more details on these errors, see this check. Results for commit ced3798. ± Comparison against base commit 4755f49. This pull request removes 57 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
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. |
924ec33 to
745e517
Compare
745e517 to
cab4422
Compare
|
Reworked and force-pushed. The three review comments about the The bulk operations now build a fresh |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Combo.java:292
- setModelItems() blocks the CHANGED closure only on the combo handle. For editable combos, the same CHANGED closure is also connected on entryHandle (see g_signal_connect_closure(entryHandle, … CHANGED)), so swapping the model / restoring active can still emit entry "changed" and produce Modify events. To keep bulk updates truly silent, block/unblock CHANGED on entryHandle too when it exists.
// Swapping the model resets the active item and emits "changed", which would
// send spurious Modify/Selection events, so keep the combo quiet meanwhile.
OS.g_signal_handlers_block_matched (handle, OS.G_SIGNAL_MATCH_DATA, 0, 0, 0, 0, CHANGED);
gtk_combo_box_toggle_wrap (false);
GTK.gtk_combo_box_set_model (handle, model);
tests/org.eclipse.swt.tests/JUnit Tests/org/eclipse/swt/tests/junit/Test_org_eclipse_swt_widgets_Combo.java:708
- The new regression test only asserts the absence of events for a READ_ONLY combo. Since bulk model swaps also affect editable combos (which have the same CHANGED closure connected on entryHandle), consider extending this test to run the same setItems/remove/removeAll sequence on an editable Combo too, so spurious Modify/Selection events on the entry widget are caught.
public void test_bulkUpdatesSendNoEventsWhenNothingIsSelected() {
// Bug 506: setItems/remove/removeAll rebuild the GTK model internally. That must
// stay invisible to applications, so an unselected combo must send no events.
String[] items = {"item0", "item1", "item2", "item3", "item4"};
Combo readOnly = new Combo(shell, SWT.READ_ONLY);
cab4422 to
7bd842b
Compare
|
Both new review points applied. Blocking CHANGED on Extending the test to editable combos was the right call too: the read-only-only version did not catch the above. Both event tests now run for Verified locally against self-built natives: all 144 |
7bd842b to
e34f976
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
bundles/org.eclipse.swt/Eclipse SWT/gtk/org/eclipse/swt/widgets/Combo.java:298
- After swapping the model in setModelItems(), the pop-up contents may get rebuilt. For SWT.RIGHT_TO_LEFT combos, other code paths explicitly reapply RTL direction to popupHandle after item mutations (e.g., add()/setItem()/setItems()), but setModelItems() doesn’t. Since removeAll() and remove(start,end) now rely on setModelItems(), RTL combos can regress to LTR in the popup after bulk updates.
gtk_combo_box_toggle_wrap (false);
GTK.gtk_combo_box_set_model (handle, model);
OS.g_object_unref (model);
if (activeIndex != -1) GTK.gtk_combo_box_set_active (handle, activeIndex);
gtk_combo_box_toggle_wrap (true);
|
New test test_removeII_keepsSelectedItemWithoutEvents fails on MacOS so as a minimum it should not be run on MacOS. |
e34f976 to
a0abd19
Compare
|
Both review points applied and force-pushed. @akurtakov right, Copilot's point about the RTL popup direction was also correct: the model swap rebuilds the popup, so the direction previously applied to its children is lost, and only |
Setting or removing a large number of combo items (>5000) was very slow on GTK: editing the GtkListStore a combo is showing costs O(n) per row, because the popup keeps its own handlers on the model and rebuilds an item for every row-inserted/row-changed. Filling or clearing a large combo was therefore quadratic. Detaching the model with gtk_combo_box_set_model(handle, 0) does not help, since the popup keeps the model even when the combo drops it. Instead setItems, removeAll and remove(start, end) now build a new GtkListStore, fill it while nothing observes it, and hand it to the combo in one step. Filling is then linear and the popup is rebuilt exactly once. Measured on GTK3 with 16000 items: setItems 41.9s -> 0.4s, removing half the items 12.2s -> 0.5s, removeAll 2.1s -> 0.1s. The model swap resets the active item and emits "changed", and on a combo with an entry restoring the selection rewrites the entry text with the value it already had. The CHANGED closure on the combo and the CHANGED, INSERT_TEXT and DELETE_TEXT closures on the entry are therefore blocked while the model is swapped, which also removes the spurious Modify events the per-row insert used to send. remove(start, end) restores the active selection, 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
a0abd19 to
ced3798
Compare
Setting or clearing a large number of
Comboitems (>5000) is very slow on GTK.Editing the
GtkListStorea combo is showing costs O(n) per row, because the popup keeps its own handlers on the model and rebuilds an item for everyrow-inserted/row-changed, so filling or clearing a large combo is quadratic.Detaching the model with
gtk_combo_box_set_model(handle, 0)does not fix this, since the popup keeps the model even when the combo drops it.Instead
setItems,removeAllandremove(start, end)now build a newGtkListStore, fill it while nothing observes it, and hand it to the combo in one step, so filling is linear and the popup is rebuilt exactly once.Measured on GTK3 with 16000 items:
setItems41.9s to 0.4s, removing half the items 12.2s to 0.5s,removeAll2.1s to 0.1s.As a side effect the combo no longer sends the spurious
Modifyevents that the per-row insert produced (one per item), because the model swap happens with theCHANGEDclosure blocked.Two new
Combotests cover the selection handling across range removals and the absence of those events.Verified locally on Linux against self-built natives, on both GTK3 and GTK4: all 143
Test_org_eclipse_swt_widgets_Combotests pass on both.Fixes #506