GNOME Bugzilla – Bug 628509
[PATCH] Script-fu loads scripts on startup by calling expensive TinyScheme interpreter commands
Last modified: 2016-10-19 08:33:34 UTC
Created attachment 169256 [details] [review] My patch with an optimization Short description: During the Script-fu's startup all Scheme scripts are loaded by calling a TinyScheme interpreter with a command: '(load "escaped/path/to/script/file")'. This fires a chain of several expensive function calls needed only to interpret the string with the command. There is a faster way - directly call functions opening and reading script file. Detailed description: During the Script-fu's startup all Scheme scripts are loaded by calling a TinyScheme interpreter with a command: '(load "escaped/path/to/script/file")' (in function script_fu_load_script() in the file: plug-ins/script-fu/script-fu-scripts.c:605) passed to an interpreter's function script_fu_run_command(). This function calls ts_interpret_string, which calls scheme_load_string, which calls Eval_Cycle, which calls opexe_0, which finally calls file_push (a function loading a file) and later (probably after popping some symbols from Scheme's stack) file_pop (closing a file). As I guessed while reading through the code, functions like script_fu_run_command, ts_interpret_string, scheme_load_string etc. are used to interpret Scheme's string, chop to atoms, recognize command, recognize number of parameters, check correctness of a command and parameters and eventually execute the command. My optimization bypasses the process of interpreting string with a command, because the command is already known in script_fu_load_script()! Instead of calling script_fu_run_command with a just-prepared-command load, it calls my function (scheme_file_push() in plug-ins/script-fu/tinyscheme/scheme.c) containing two calls: file_push() and file_pop(). Startup speed-up: On my laptop the part of GIMP startup when "extension-script-fu" text is visible takes approx. 1,65 s with the GIMP from git and 1,04 s with my patch. Callgrind graphs: The difference in complexity of loading scripts are shown on graphs: http://students.mimuw.edu.pl/~lc277642/gimp/bugzilla/callgrind.out.absolute_cost.before.jpg http://students.mimuw.edu.pl/~lc277642/gimp/bugzilla/callgrind.out.absolute_cost.after.jpg Explanation to the graphs: One can notice that in "after" graph there is no call to script_fu_find_scripts from script_fu_run. That's only because this call was so unimportant, that Callgrind didn't show it. The difference in calculation complexity of functions tinyscheme_init and script_fu_find_scripts called from script_fu_run can be compared here: Before: http://students.mimuw.edu.pl/~lc277642/gimp/bugzilla/callgrind.out.screen.absolute_cost.before.png After: http://students.mimuw.edu.pl/~lc277642/gimp/bugzilla/callgrind.out.absolute_cost.after.jpg The difference in tinyscheme_init is very small and could be just a measurement error, while a difference in script_fu_find_scripts (and functions called from this one) is huge. My patch is in attachment. Hope to get it approved by you soon.
The fourth link ("After:") should be: http://students.mimuw.edu.pl/~lc277642/gimp/bugzilla/callgrind.out.screen.absolute_cost.after.png
Thank you for the patches. You are right about passing a load command to the TinyScheme interpreter as not being the most efficient way to load scripts. It isn't immediately obvious that using file_push and file_pop will force loading of a script file. There is also some minor overhead with the push and pop operation. Have you tried to use the ts_load_file() function which is available in scheme_wrapper.c? ts_load_file() is used to load the files needed during initialization of TinyScheme.
As it was asked in http://www.mail-archive.com/gimp-developer@lists.xcf.berkeley.edu/msg08946.html: Where can I find TinyScheme's documentation? I hope that the answer that can be found on your website (http://www.ve3syb.ca/software/tinyscheme/index.html) - "The biggest problem I have had with TinyScheme has been the lack of any documentation(...)" isn't true... As I couldn't find any documentation, all I know about TinyScheme comes from my looking at the code, reading it, traces in gdb and skimming through the names of functions. I've seen a function scheme_load_file, which is called by a ts_load_file() function, but because I didn't know what this function does, simply I didn't use it. Could you please provide me with some documentation? In the mean time, I will try to use ts_load_file.
It seems that calling ts_load_file results in the same as the original source - the interpreter is being called (calls to such functions as EvalCycle and in result opexe_0 and opexe_5 are made) and script-fu loads the same long as without changing anything. Please take a look at: http://students.mimuw.edu.pl/~lc277642/gimp/bugzilla/callgrind.out.23602. You can open the file with KCacheGrind. Take a look at Call Graph. It looks the same as in the original GIMP: http://students.mimuw.edu.pl/~lc277642/gimp/bugzilla/callgrind.out.absolute_cost.before.jpg Looking at the graph, it seems that the most expensive are calls to scheme_load_file -> Eval_Cycle -> opexe_5 and going deeper in call trace: -> mk_atom -> mk_symbol -> oblist_find_by_name -> utf8_stricmp. I can optimize a bit utf8_stricmp (I have sent once to a gimp-dev list a patch with mixed two optimizations - loading files and utf8_stricmp), nevertheless it will be far faster to bypass the whole interpreter mechanism! By the way, could you please tell me what such functions as mk_symbol, mk_atom and oblist_find_by_name actually do?
Regarding comment #3: The source code is the documentation. If there was any other written documentation it, or a link to it, would be on my website. Regarding comment #4: EvalCycle has to be invoked at some point when GIMP is starting up and it is loading scripts. At a minimum, the script files have to be read and executed to do the script-fu-register() call to create the menu entries. Questions about mk_symbol, mk_atom, oblist_find_by_name, or other things related to TinyScheme and how it operates should be asked on the GIMP developer IRC channel, the GIMP developer mailing list, or the TinyScheme mailing list.
Reopening as I think the questions in comment #2 are answered by now.
So... can we expect this patch to have a noticeable effect on script-file loading / GIMP startup?
Ok so for info, the patch does not apply cleanly on master. I tried to fix it a little so that it applies, but then Script-fu scripts were not loaded properly. I didn't investigate further because I can't make the time right now for this. It would be good if someone who actually cares and understand more this part of the code could review this. It is kind of sad that a 2010 patch (about 10 line change only) improving startup time never made it to our codebase. :-/
Well, I've always read the previous comments as 'this needs more work to be appliable'.
Well I was not sure if they were just either asking for more information on the changes and just asking if other alternatives were considered, or indeed requesting more work on this approach. But reading again comment 5, it actually looks like it is requesting for more work on the patch. That's true. Anyone who wants to see if anything can still be salvaged from the original patch or idea?
At least we should have a better state for that patch than "none" (aka "unreviewed"). Kevin, what is the appropriate way to proceed?
In comment #8 Jehan said that with the change proposed the scripts didn't load properly. If that meant the menu entries for the scripts were missing that doesn't surprise me. In comment #5 I mentioned that the scripts need to be executed or else the menu entries for the scripts won't be added to the GIMP menus. The way to proceed would be for me to complete the changes to Tiny-Fu2 to allow Script-Fu scripts to work more like other plug-ins. They would only need to be executed during startup if they had changed since the last run of GIMP. I've been busy working on other projects these days and haven't been motivated to work on Tiny-Fu2.
Really, how "expensive" can this be? In the end, we are still doing disk I/O which is probably much more expensive. Also, this is not about "script loading is slow", it's about "let's optimize". I'd say if it's not actually slow don't optimize it, IIRC there have been no complaints about script loading being slow, have there? If not, I propose WONTFIX.
In any case, nobody seems to have been motivated to work on this for years. So let's close it and if someone wants to bring this to life, fix this patch or make a new one, they can always reopen or make a new report.
So be it.