Hello,

W dniu 31 sierpnia 2010 00:39 użytkownik Sven Neumann <s...@gimp.org>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
_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Reply via email to