Replace gtk_combo_box_text_insert_text with direct model insertion

[imciner2]

There is a lot of overhead in the gtk_combo_box_text_insert_text function, so adding a lot of items to a choicebox can be an expensive operation when it is used. We have encountered this in KiCad when trying to use a wxChoiceBox as a font chooser on systems that have a lot of fonts installed (https://gitlab.com/kicad/code/kicad/-/issues/14277).

I traced back some of the slow performance on creation of the wxChoiceBox to the method that was being used to add the items to the combobox. Experiments showed that adding 10000 items to the choicebox takes 6.75s in the current implementation using gtk_combo_box_text_insert_text. The other way of adding items is to directly access the underlying data model using gtk_list_store_insert_with_values. Using the data model access, experiments showed the time needed to add 10000 items dropped to 438.2ms.

It turns out, that was already the method used in wxBitmapComboBox, so this actually collapses that down into the base wxChoice class, removing the overriden function from wxBitmapComboBox. I tried using the same 2-step insertion pattern from wxBitmapComboBox (calling gtk_list_store_insert followed by gtk_list_store_set_value, but it turns out this is just as slow as the original method, and using the 1-step method by calling gtk_list_store_insert_with_values was much faster).

I have split this into two separate commits. The first is ABI-compatible, so it would be good to backport it to 3.2. The second removes the now redundant function from wxBitmapComboBox, so it changes the ABI (but leaves the API). I would have preferred to just collapse out GTKInsertComboBoxTextItem entirely so each item didn't have to query for the model, but that would remove that from the API, and it could theoretically be being overriden by user's in their own implementations currently.

This doesn't completely solve the performance issues for the large choicebox though, there is still a large performance hit when computing the text size and showing the menu, but that needs to be handled separately (and I haven't quite figured out the best way to do that yet).

These experiments can be seen using the minimal sample with this diff:

diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
index 501caf9096..428f3db1b5 100644
--- a/samples/minimal/minimal.cpp
+++ b/samples/minimal/minimal.cpp
@@ -144,6 +144,9 @@ MyFrame::MyFrame(const wxString& title)
     // set the frame icon
     SetIcon(wxICON(sample));
 
+    wxBoxSizer* main_sizer = new wxBoxSizer(wxVERTICAL);
+    SetSizer(main_sizer);
+
 #if wxUSE_MENUBAR
     // create a menu bar
     wxMenu *fileMenu = new wxMenu;
@@ -162,12 +165,13 @@ MyFrame::MyFrame(const wxString& title)
     // ... and attach this menu bar to the frame
     SetMenuBar(menuBar);
 #else // !wxUSE_MENUBAR
+
     // If menus are not available add a button to access the about box
     wxSizer* sizer = new wxBoxSizer(wxHORIZONTAL);
     wxButton* aboutBtn = new wxButton(this, wxID_ANY, "About...");
     aboutBtn->Bind(wxEVT_BUTTON, &MyFrame::OnAbout, this);
     sizer->Add(aboutBtn, wxSizerFlags().Center());
-    SetSizer(sizer);
+    main_sizer->Add(sizer);
 #endif // wxUSE_MENUBAR/!wxUSE_MENUBAR
 
 #if wxUSE_STATUSBAR
@@ -175,6 +179,18 @@ MyFrame::MyFrame(const wxString& title)
     CreateStatusBar(2);
     SetStatusText("Welcome to wxWidgets!");
 #endif // wxUSE_STATUSBAR
+
+    wxArrayString labels;
+
+    for( int i=0; i < 10001; i++)
+    {
+        labels.Add( wxString::Format( "Item %d", i ) );
+    }
+
+    wxChoice* choice = new wxChoice(this, wxID_ANY, wxDefaultPosition, wxDefaultSize, labels);
+    main_sizer->Add(choice);
+
+    Layout();
 }

The flamegraph from the original implementation is:

The flamegraph from the proposed implementation is (note the different scale on the time axis):

[vadz]

Thanks for improving this!

We can definitely merge this already or you could address the 2 minor remarks below if you'd like.

Concerning the API breakage: functions prefixed with GTK are specifically meant to be private and not part of the public API and so we can remove it without problem. Unfortunately we do need to keep it in 3.2 to avoid breaking the ABI...

[imciner2]

Concerning the API breakage: functions prefixed with GTK are specifically meant to be private and not part of the public API and so we can remove it without problem.

Thanks for clarifying this. I have gone ahead and removed it from the class then, collapsing everything into DoInsertItems. It is a separate commit, so backporting the core of this change to 3.2 can still be done by taking the first commit and its fixup.

[vadz]

Thanks for the updates! I think it would be better if you rebased the commits here, so that I could cherry-pick just a single commit to 3.2.

[imciner2]

Ok, I have rebased everything so now only 1 commit (the first) needs to be cherry-picked, everything else is master-only.

[vadz]

Great, thanks! Will merge (and backport) soon.