#1 Always destroy open and save dialog after use

closed-fixed
interface (8)
5
2011-06-10
2011-06-05
Matthew Brush
No

Discussion

  • Enrico Tröger
    Enrico Tröger
    2011-06-05

    • labels: --> interface
    • assigned_to: nobody --> eht16
     
  • Enrico Tröger
    Enrico Tröger
    2011-06-05

    Not sure whether we want this unconditionally.

    There is a reason why we cache the whole dialog: speed.

    I did some basic measurements on my system. The absolute values are irrelevant but notice the differences. The time values specify entering and leaving dialogs_show_open_file(), see also the attached patch which describes what I did:

    Re-using the dialog (#define REUSE 1):

    Geany-INFO: dialogs_show_open_file(): 0.208
    Geany-INFO: dialogs_show_open_file(): 0.013
    Geany-INFO: dialogs_show_open_file(): 0.013
    Geany-INFO: dialogs_show_open_file(): 0.012
    Geany-INFO: dialogs_show_open_file(): 0.012
    Geany-INFO: dialogs_show_open_file(): 0.012
    Geany-INFO: dialogs_show_open_file(): 0.013
    Geany-INFO: dialogs_show_open_file(): 0.013
    Geany-INFO: dialogs_show_open_file(): 0.012
    Geany-INFO: dialogs_show_open_file(): 0.012
    Geany-INFO: dialogs_show_open_file(): 0.013

    Create/Destroy the dialog everytime (without REUSE):

    Geany-INFO: dialogs_show_open_file(): 0.201
    Geany-INFO: dialogs_show_open_file(): 0.167
    Geany-INFO: dialogs_show_open_file(): 0.160
    Geany-INFO: dialogs_show_open_file(): 0.136
    Geany-INFO: dialogs_show_open_file(): 0.149
    Geany-INFO: dialogs_show_open_file(): 0.167
    Geany-INFO: dialogs_show_open_file(): 0.135
    Geany-INFO: dialogs_show_open_file(): 0.140
    Geany-INFO: dialogs_show_open_file(): 0.131
    Geany-INFO: dialogs_show_open_file(): 0.154
    Geany-INFO: dialogs_show_open_file(): 0.168

    As you can easily see, the current behaviour (first set) is slow on the first call, obviously because the dialog has to be created. But subsequent calls to the cached dialog instance which only show it, are noticeable faster.
    The second set is with destroying and re-creating the dialog everytime. The initial creation is still slower than subsequent calls. However, comparing the calls of both sets show that the current behaviour opens the dialog faster, about ten times faster. As said, the absolute values obviously have no meaning but it's likely that the difference is similar on other machines. My system is Debian Testing on an AMD Athlon 64 X2 Dual 4600+. Not that fast but also not that slow. I guess the difference is more noticeable on older machines where Geany wants to run as well.

    And yes, caching the dialog for performance costs a little more memory, of course.

    Conclusion:
    we could use the (create/destroy part of the) patch ifdef'ed for affected GTK versions (maybe 2.20 to 2.24 but I didn't verify this yet). This would be a huge and ugly hack but would fix it at least for those users running the affected GTK versions.
    Not sure about the other part of the patch, basically I'd say it's way too much for a workaround, on the other hand it's quite confusing if suddenly the state tracking is missing just because the user has to use a buggy GTK version.

     
  • @eht16: I'm not completely sure about the measurements: yes, it is faster to simply show, but while the GtkFileSystemModel (or whatever GTK representation) exists, it will monitor the directory for changes, which will take some CPU cycles -- maybe more than what we would spend to re-create it every time.

    So yes, the idea was for a workaround, but I'm not sure we would actually loose CPU cycles… though I've no idea how to measure this.

     
  • Enrico Tröger
    Enrico Tröger
    2011-06-05

    True, I didn't think of the filesystem monitoring. This is new since we made decision about caching the dialog long time ago.
    So, yes, it's probably better to not keep the dialogs and recreate it everytime we need it.

    I probably won't manage to review and commit the patch within the next days, so Colomban, if you like go ahead. If not, I'll do later, probably after June 13th.

     
  • Enrico Tröger
    Enrico Tröger
    2011-06-05

    • assigned_to: eht16 --> nobody
     
    • assigned_to: nobody --> colombanw
     
    • status: open --> closed-fixed
     
  • Well, the patch had quite a few problems in the state saving side (state saving for the encoding was wrong, and there was no state saving of the file filter or the expander), but the idea itself is good (as already pointed by me and Enrico).

    I rewritten most of patch and a lot of code around, but should be finally fixed.
    For reference, commit is r5837.

    Thanks for the patch, and especially pushing us to fix it ^^