GNOME Bugzilla – Bug 330099
Startup: Data should be loaded and unloaded in a lazy manner
Last modified: 2018-05-24 11:43:56 UTC
Attached to this bug is the recent version of my patch to make sure that data in the GIMP is loaded and unloaded in a lazy manner. This patch adds two function to GIMP_DATA - gimp_data_load() and gimp_data_unload() for signifying when the data of the object should be loaded to memory. This patch makes sure that only the essential information for the data is loaded, and the rest is loaded from the disk when it is requested.
Created attachment 58796 [details] [review] The relevant patch - revision 9. This is revision 9 of the patch. It is possible that some loading functions for sub-types of data were not converted yet.
Created attachment 58822 [details] [review] The relevant patch - revision 12 This is a newer version of the patch. It has a problem that when GIMP exits it generates a lot of warnings. I'll try to investigate further.
Created attachment 58850 [details] [review] Patch revision 13 This patch eliminates the warnings that appeared in revision 12, and also fixes the bugs that caused them.
Created attachment 58865 [details] [review] Patch revision 14 This new revision of the patch fixes many GError-related problems. Thanks to Sven for pointing me one of them.
fprintf (stderr, foo) must not be used. Printing error messages to stderr is fine for debugging and perhaps for the impossible error, but it should not be used in general because the casual user will never see any error message printed there. If you want to print error messages to stderr, please use g_printerr() instead. g_printerr() takes care of doing the charset conversion to the user's locale in case it isn't an UTF-8 locale.
Finally, some comments :-) - GimpBrush::load/unload() should probably be called data_load() and data_unload() because that's how the functions are called and it makes the purpose clearer. - gimp_data_load()/unload() should be called use() and unuse() because that's what they do, changing the use count and not loading/unloading. - the GimpBrushType enum should be typedefed as all out enums, live in app/core-enums.h and used as such in the GimpBrush struct (instead of gint) - instead of g_error_free(error), it's safer to use g_clear_error(&error) because it sets the error variable to NULL so it's ready for being reused. - gimp_data_unload() needs g_return_if_fail(load_count > 0) - in order to jump in one's eyes, the TODOs in the code should be: #ifdef __GNUC__ #warning FIXME foobar #endif - there are still some incomplete prototypes - some function headers are indented like prototypes - please let's get rid of these perl utility functions in the pdb files and put the duplicated code back. pdbgen files are hard enough to read already. (e.g. _get_gradient_code() and friends)
The patch introduces file offsets and I don't really understand how this is supposed to work. There seems to be an assumption made here that the files on disk would not change. This is a dangerous assumption to make as of course the data files may be erased or changed while GIMP is running. The data files would thus have to be reloaded completely. Of course this might fail. So the approach we are currently taking introduces a lot of error handling. Currently we can assume the integrity of the data objects in all places. The data can only change or vanish if the user hits the Reload button. Perhaps it isn't such a good idea to change this. So I would like to propose another approach which might turn out to be easier to get right. The idea is to use something very similar to the GTK+ icon cache. For each of the data directories in a data types search path, there would be a data cache on disk. This cache file would contain the data of all data files in that particular directory. It would contain this data in a form that can directly be mmap'ed (using GMappedFile). On startup and whenever the user hits Reload, GIMP would check the integrity of the cache file by comparing the stored file sizes and modification times to the files on disk. If a file on disk has been changed, deleted or added, the cache is regenerated. For the code in gimp this means less changes because all users of GimpData could continue to assume that the data is there and loaded. We would still reduce our memory footprint and startup time because all the data files would just be mmap'ed. In order to reduce the time for the very first start, the global data directories could already contain data cache files that are generated at installation time.
Another point of reference would be to look at fontconfig HEAD. They've implemented cache files via mmap.
It seems that there are too many issues remaining for this to be implemented in 2.4, which is going to be released very shortly, so I am bumping the target to Future. Hopefully the not-too-distant-Future.
*** Bug 526122 has been marked as a duplicate of this bug. ***
*** Bug 543940 has been marked as a duplicate of this bug. ***
*** Bug 557864 has been marked as a duplicate of this bug. ***
*** Bug 594349 has been marked as a duplicate of this bug. ***
*** Bug 608313 has been marked as a duplicate of this bug. ***
*** Bug 682348 has been marked as a duplicate of this bug. ***
So, does this patch work with 2.8? It looks like it's old. Or is this just an issue you are currently trying to just implement in a future version?
(In reply to comment #16) > So, does this patch work with 2.8? It looks like it's old. Or is this just an > issue you are currently trying to just implement in a future version? I have no idea if this patch works with 2.8. Maybe it does not. In any case, I have written it for a much older version, and it was deemed unsuitable due to the fact that I didn't use mmap() and I didn't write a new version based on mmap().
*** Bug 704135 has been marked as a duplicate of this bug. ***
We have many bug reports on this. Often Windows users report that GIMP takes so long to startup that Windows assumes GIMP is dead and reports a crash As this is not a pure nice-to-have-enhancement, but an old, known problem and there has been existing a patch for more than seven years, we should really make the patch matching to the latest API and apply it. Thus increasing the severity and setting the milestone to 2.8.
May I ask you a question? Sven, there was a call that GIMP is searching Windows developers to improve the experience of the Windows version. Have there been any afforts?
Of course you may ;-) I heard that call, too, and decided to support the GIMP help team here. In the meantime a lot of tasks has come up and fills me, like maintenance of the continuous build server (Jenkins), wiki work, testing, bug reporting and triaging and mailing list discussions. So, I'm personally quite busy and the work I originally wanted to do is having a slow progress. (Yes, I'm a bit sad about it) There were some other short timed attempts to contribute Windows-related code to GIMP which seem to have already ended. In the meantime some other hopeful contributors appeared, for instance Téo and Jehan, who are working on GIMP in general (at least their efforts also benefit the Windows users). To summarize it: yes, we have done something to improve the situation for Windows, but still any help is welcome.
Hello, 2 questions: 1/ so is the proposed right implementation supposed to be done with mmap then? 2/ I see you are also writing about slowness on Windows. But isn't mmap a UNIX/POSIX API? If so, the lazy-loading implementation would not pertain to the Windows version anyway. Or I misunderstood something? Thanks.
This is clearly not making it into 2.8, and not into 2.10 either unless some miracle happens.
*** Bug 792313 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gimp/issues/178.