Hello,

I have made several optimizations in loading script-fu extension making the loading process a bit faster. It's the first time I change something in an open source program and don't know best way to "announce" it and get it reviewed, so I send a patch in a mail. The major speedup is caused by calling my function load_script_file instead of script_fu_run_command("(load ...."). Please review my changes and send an answer if the patch is OK.

diff -rup /home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/scheme-wrapper.c gimp/plug-ins/script-fu/scheme-wrapper.c ---
/home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/scheme-wrapper.c 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.c 2010-08-24 22:47:21.000000000 +0200
@@ -298,6 +298,14 @@ ts_interpret_stdin (void) scheme_load_file (&sc, stdin);
}

+/* ==== modified by Lukasz Czerwinski (lc277642@students.mimuw.edu.pl) */ +
+int load_script_file(const char *fname) { + return scheme_file_push(&sc,fname); +}
+
+/* ==== end of modification ==== */ +
gint
ts_interpret_string (const gchar *expr) {
diff -rup
/home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/scheme-wrapper.h gimp/plug-ins/script-fu/scheme-wrapper.h ---
/home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/scheme-wrapper.h 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.h 2010-08-24 23:09:18.000000000 +0200
@@ -32,6 +32,10 @@ const gchar * ts_get_success_msg (v

void ts_interpret_stdin (void);

+/* ==== modified by Lukasz Czerwinski (lc277642@students.mimuw.edu.pl) */ +int load_script_file(const char *fname); +/* ==== end of modification ==== */ +
/* if the return value is 0, success. error otherwise. */ gint ts_interpret_string (const gchar *expr);

diff -rup /home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/script-fu-scripts.c gimp/plug-ins/script-fu/script-fu-scripts.c ---
/home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/script-fu-scripts.c 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/script-fu-scripts.c 2010-08-25 16:58:31.000000000 +0200
@@ -589,6 +589,7 @@ script_fu_run_command (const gchar *com return success;
}

+/* modified by Lukasz Czerwinski (lc277642@students.mimuw.edu.pl) */ static void
script_fu_load_script (const GimpDatafileData *file_data, gpointer user_data) @@ -602,13 +603,15 @@ script_fu_load_script (const GimpDatafil command = g_strdup_printf ("(load \"%s\")", escaped); g_free (escaped);

- if (! script_fu_run_command (command, &error)) +// if (! script_fu_run_command (command, &error)) + if (! load_script_file(file_data->filename)) {
gchar *display_name = g_filename_display_name (file_data->filename);
gchar *message = g_strdup_printf (_("Error while loading %s:"),
display_name); + g_message (message);

- g_message ("%s\n\n%s", message, error->message); +// g_message ("%s\n\n%s", message, error->message);

g_clear_error (&error); g_free (message);
@@ -625,6 +628,7 @@ script_fu_load_script (const GimpDatafil g_free (command);
}
}
+/* end of modifications */

/* * The following function is a GTraverseFunction. diff -rup
/home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/tinyscheme/scheme.c gimp/plug-ins/script-fu/tinyscheme/scheme.c ---
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/tinyscheme/scheme.c 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/tinyscheme/scheme.c 2010-08-25 17:08:44.000000000 +0200
@@ -125,6 +125,21 @@ static int utf8_stricmp(const char *s1, return result;
}

+/* modified by Lukasz Czerwinski (lc277642@students.mimuw.edu.pl) */ +#define stricmp_left_converted utf8_stricmp_left_converted +static int utf8_stricmp_left_converted(const char *case_folded, const char *s2)
+{
+ char *t2a;
+ int result;
+
+ t2a = g_utf8_casefold(s2, -1);
+
+ result = strcmp(case_folded, t2a); + g_free(t2a);
+ return result;
+}
+/* end of modification */
+
#define min(a, b) ((a) oblist));
for (x = vector_elem(sc->oblist, location); x != sc->NIL; x = cdr(x)) { s = symname(car(x));
/* case-insensitive, per R5RS section 2. */ - if(stricmp(name, s) == 0) {
+ if(stricmp_left_converted(name_case_folded, s) == 0) { + g_free(name_case_folded);
return car(x);
}
}
+ g_free(name_case_folded);
return sc->NIL;
}

@@ -1415,6 +1433,14 @@ static void finalize_cell(scheme *sc, po

/* ========== Routines for Reading ========== */

+/* modified by Lukasz Czerwinski (lc277642@students.mimuw.edu.pl) */ +int scheme_file_push(scheme *sc, const char *fname) { + int ret = file_push(sc, fname);
+ file_pop(sc);
+ return ret;
+}
+/* end of modification */
+
static int file_push(scheme *sc, const char *fname) { FILE *fin = NULL;
if (sc->file_i == MAXFIL-1)
diff -rup
/home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/tinyscheme/scheme.h gimp/plug-ins/script-fu/tinyscheme/scheme.h ---
/home/lukasz/OPOS/SOURCE/gimp-git-patched/gimp/plug-ins/script-fu/tinyscheme/scheme.h 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/tinyscheme/scheme.h 2010-08-24 23:07:28.000000000 +0200
@@ -152,6 +152,11 @@ SCHEME_EXPORT pointer scheme_eval(scheme void scheme_set_external_data(scheme *sc, void *p); SCHEME_EXPORT void scheme_define(scheme *sc, pointer env, pointer symbol, pointer value);

+/* modified by Lukasz Czerwinski (lc277642@students.mimuw.edu.pl) */ +SCHEME_EXPORT int scheme_file_push(scheme *sc, const char *fname); +/* end of modification */
+
+
typedef pointer (*foreign_func)(scheme *, pointer);

pointer _cons(scheme *sc, pointer a, pointer b, int immutable);

Looking forward to your answer, ?ukasz Czerwi?ski

Hello,

W dniu 31 sierpnia 2010 00:39 u?ytkownik Sven Neumann napisa?:

On Mon, 2010-08-30 at 23:31 +0200, ?ukasz Czerwi?ski wrote:

I have made several optimizations in loading script-fu extension making the loading process a bit faster. It's the first time I change something in an open source program and don't know best way to "announce" it and get it reviewed, so I send a patch in a mail. The major speedup is caused by calling my function load_script_file instead of script_fu_run_command("(load ...."). Please review my changes and send an answer if the patch is OK.

Thanks for your patch. It definitely needs some work though. First of all it is not OK to comment out code. If code should be removed, then please remove it.

Ok, I will remove.

Passing a string literal directly to g_message() is also not a good idea. Please use g_message("%s", message) instead. But I didn't understand what this part of your patch is trying to achieve.

Why it's not a good idea? What's the difference? Nevertheless, ok, I'll change it.

Overall it would help a lot if you could explain what your patch tries to achieve and what the purpose of the changes are. I have not been able to understand the benefits of your changes just by looking at the patch.

The difference is clearly visible on a callgrind's graph. Compare flow between script_fu_run and Eval_Cycle at http://students.mimuw.edu.pl/~lc277642/gimp/before.jpg and http://students.mimuw.edu.pl/~lc277642/gimp/after.jpg. Full versions of the graphs in jpg and Callgrind formats are available at http://students.mimuw.edu.pl/~lc277642/gimp/.

A description of an optimization: all script-fu scripts loaded on startup was loaded using a command "(load ...)" of a TinyScheme language. This means running several functions escaping a path to a script, interpreting the string, choping it to tokens, analyzing, unescaping and finally loading the script. My optimization makes Gimp call directly a function loading a script without using a slow (comparing to calling one function in native C) TinyScheme interpreter. Instead I've written two helper functions ( load_script_file and scheme_file_push) responsible for loading scripts.

I have done also a second modification - connected with comparing strings (stricmp), but I've just spotted an error in it, so I'll fix it and send it later.

Is my description detailed enough or I should add something?

The corrected patch:

diff -rup /home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/scheme-wrapper.c gimp/plug-ins/script-fu/scheme-wrapper.c ---
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/scheme-wrapper.c 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.c 2010-08-31 13:49:59.000000000 +0200
@@ -298,6 +298,10 @@ ts_interpret_stdin (void) scheme_load_file (&sc, stdin);
}

+int load_script_file(const char *fname) { + return scheme_file_push(&sc,fname); +}
+
gint
ts_interpret_string (const gchar *expr) {
diff -rup
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/scheme-wrapper.h gimp/plug-ins/script-fu/scheme-wrapper.h ---
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/scheme-wrapper.h 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.h 2010-08-31 13:50:17.000000000 +0200
@@ -32,6 +32,8 @@ const gchar * ts_get_success_msg (v

void ts_interpret_stdin (void);

+int load_script_file(const char *fname); +
/* if the return value is 0, success. error otherwise. */ gint ts_interpret_string (const gchar *expr);

diff -rup /home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/script-fu-scripts.c gimp/plug-ins/script-fu/script-fu-scripts.c ---
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/script-fu-scripts.c 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/script-fu-scripts.c 2010-08-31 13:51:33.000000000 +0200
@@ -602,13 +602,12 @@ script_fu_load_script (const GimpDatafil command = g_strdup_printf ("(load \"%s\")", escaped); g_free (escaped);

- if (! script_fu_run_command (command, &error)) + if (! load_script_file(file_data->filename)) {
gchar *display_name = g_filename_display_name (file_data->filename);
gchar *message = g_strdup_printf (_("Error while loading %s:"),
display_name); -
- g_message ("%s\n\n%s", message, error->message); + g_message ("%s", message);

g_clear_error (&error); g_free (message);
@@ -625,6 +624,7 @@ script_fu_load_script (const GimpDatafil g_free (command);
}
}
+/* end of modifications */

/* * The following function is a GTraverseFunction. diff -rup
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/tinyscheme/scheme.c gimp/plug-ins/script-fu/tinyscheme/scheme.c ---
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/tinyscheme/scheme.c 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/tinyscheme/scheme.c 2010-08-31 13:52:39.000000000 +0200
@@ -1415,6 +1415,12 @@ static void finalize_cell(scheme *sc, po

/* ========== Routines for Reading ========== */

+int scheme_file_push(scheme *sc, const char *fname) { + int ret = file_push(sc, fname);
+ file_pop(sc);
+ return ret;
+}
+
static int file_push(scheme *sc, const char *fname) { FILE *fin = NULL;
if (sc->file_i == MAXFIL-1)
diff -rup
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/tinyscheme/scheme.h gimp/plug-ins/script-fu/tinyscheme/scheme.h ---
/home/lukasz/OPOS/SOURCE/gimp-git-original/gimp/plug-ins/script-fu/tinyscheme/scheme.h 2010-08-24
17:49:15.000000000 +0200
+++ gimp/plug-ins/script-fu/tinyscheme/scheme.h 2010-08-31 13:53:08.000000000 +0200
@@ -152,6 +152,9 @@ SCHEME_EXPORT pointer scheme_eval(scheme void scheme_set_external_data(scheme *sc, void *p); SCHEME_EXPORT void scheme_define(scheme *sc, pointer env, pointer symbol, pointer value);

+SCHEME_EXPORT int scheme_file_push(scheme *sc, const char *fname); +
+
typedef pointer (*foreign_func)(scheme *, pointer);

pointer _cons(scheme *sc, pointer a, pointer b, int immutable);

Best wishes, ?ukasz Czerwi?ski

2010/9/1 Kevin Cozens

?ukasz Czerwi?ski wrote:

I have made several optimizations in loading script-fu extension making

the

loading process a bit faster.

Thank you for the patches. They will be reviewed son. I can't look at them for another day or two as I'm finishing up some changes on another project.

I would ask that you update the patches to address the issues raised by Sven and then post a bug report against Script-Fu. Attach the patches to the report instead of pasting them in the body of the report. The bug report will prevent this issue from getting lost in my e-mail's mail folders.

It would help if you could also send some additional information about the patches to this mailing list to explain the changes. I noticed in the before and after graphics that your modified version doesn't go through the usual steps to locate the script files which leaves me wondering how it finds what scripts need to be loaded.

It seems that you and Sven asked me to make two different actions and it's useless to do both. Sven asked me to commit my patch into git, you have told me to create a bug and add a patch there as an attachment. Whose commands should I obey? :] I'm trying to guess what's the procedure for making changes in code and get them approved by some more experience developers. Could you provide me such information? Maybe there should be a page on the website saying what should be done step by step to patch GIMP and get the patch approved. I'm thinking of a small guide for newbie developers. By now I have submitted a bug report for Script-fu in GIMP, but didn't commit my changes into git. Sven, Kevin, should I do it?

As for your request for additional information, a description of my change I have written in a description of my bug report: https://bugzilla.gnome.org/show_bug.cgi?id=628509

Looking forward to your answer, ?ukasz Czerwi?ski