Sent: 2010-08-30 23:31:26 UTC (over 1 year ago)

From: ?ukasz Czerwi?ski

Patch shortening GIMP startup time

permalink

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

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-08-31 00:39:32 UTC (over 1 year ago)

From: Sven Neumann

Patch shortening GIMP startup time

permalink

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.

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.

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.

Sven

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-08-31 14:18:13 UTC (over 1 year ago)

From: ?ukasz Czerwi?ski

Patch shortening GIMP startup time

permalink

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

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-08-31 15:15:50 UTC (over 1 year ago)

From: Michael Natterer

Patch shortening GIMP startup time

permalink

On Tue, 2010-08-31 at 14:18 +0200, ?ukasz Czerwi?ski wrote:
> >
> > 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.

Because if you write g_message(string) and string happens to be
"Format %s string %d attack %s", you are screwed :-)

ciao,
--mitch

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-08-31 19:23:59 UTC (over 1 year ago)

From: Sven Neumann

Patch shortening GIMP startup time

permalink

On Tue, 2010-08-31 at 14:18 +0200, ?ukasz Czerwi?ski wrote:

> 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.

It's potentially dangerous. g_message() takes a format string as the
first parameter. Of course you can pass a literal string to it. But in
case that literal string contains printf-like format directive, your
code will crash. Since we can't safely assume that the error message
will definitely not contain such format directives, it is safer to use
"%s" as the format string.

> 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.

It would be best if you could start to use git for your changes. You can
then make separate commits for each of your changes and submit your
changes broken into smaller commits. That makes review a lot easier. It
is also easier for us to apply your patches then.

Sven

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-09-01 07:58:20 UTC (over 1 year ago)

From: Kevin Cozens

Patch shortening GIMP startup time

permalink

?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.

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-09-01 15:53:34 UTC (over 1 year ago)

From: ?ukasz Czerwi?ski

Patch shortening GIMP startup time

permalink

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

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-09-01 16:04:00 UTC (over 1 year ago)

From: Michael Schumacher

Patch shortening GIMP startup time

permalink

-------- Original-Nachricht --------
> Datum: Wed, 1 Sep 2010 15:53:34 +0200
> Von: "?ukasz Czerwi?ski"

> 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? :]

Both, they aren't conflicting.

You probably get the GIMP source code from Git already, as described here:
http://live.gnome.org/Git/Developers

Contributing patches is described there as well:
http://live.gnome.org/Git/Developers#Contributing_patches

HTH,
Michael

--
GMX DSL SOMMER-SPECIAL: Surf & Phone Flat 16.000 für nur 19,99 €/mtl.!*
http://portal.gmx.net/de/go/dsl
_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer

Sent: 2010-09-01 18:06:15 UTC (over 1 year ago)

From: Kevin Cozens

Patch shortening GIMP startup time

permalink

?ukasz Czerwi?ski wrote:
> It seems that you and Sven asked me to make two different actions and it's
> useless to do both.

In the e-mail messages I saw you being told of some issues with the patch
but not a one for you to the changes to git. The bug report ensures this
issue and the patches won't get lost or forgotten.

> I'm trying to guess what's the procedure for making
> changes in code and get them approved by some more experience developers.

You have been doing the right things. You started by sending an e-mail to
the GIMP developer mailing list. Even better, you provided a set of patches.

As I am the main person who deals with issues related to the Script-Fu
component of GIMP, Sven made sure I was aware of the mail you sent and
asked me to follow up on it.

If you want to discuss things further, you can find myself, Sven, and other
GIMP developers on the #gimp channel on IRC. For more information about
GIMP related IRC channels go to http://www.gimp.org/irc.html.

> As for your request for additional information, a description of my change I
> have written in a description of my bug report:

Thank you.

_______________________________________________
Gimp-developer mailing list
Gimp-developer@lists.XCF.Berkeley.EDU
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer