Re: [Gimp-developer] Patch shortening GIMP startup time
Ł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
Re: [Gimp-developer] Patch shortening GIMP startup time
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
Re: [Gimp-developer] Patch shortening GIMP startup time
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
Re: [Gimp-developer] Patch shortening GIMP startup time
Ł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
Re: [Gimp-developer] Patch shortening GIMP startup time
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
Re: [Gimp-developer] Patch shortening GIMP startup time
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
Re: [Gimp-developer] Patch shortening GIMP startup time
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.0 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.c 2010-08-31 13:49:59.0
+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.0 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.h 2010-08-31 13:50:17.0
+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.0 +0200
+++ gimp/plug-ins/script-fu/script-fu-scripts.c 2010-08-31
13:51:33.0 +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
---
/
Re: [Gimp-developer] Patch shortening GIMP startup time
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
[Gimp-developer] Patch shortening GIMP startup time
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.0 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.c 2010-08-24 22:47:21.0
+0200
@@ -298,6 +298,14 @@ ts_interpret_stdin (void)
scheme_load_file (&sc, stdin);
}
+/* modified by Lukasz Czerwinski (lc277...@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.0 +0200
+++ gimp/plug-ins/script-fu/scheme-wrapper.h 2010-08-24 23:09:18.0
+0200
@@ -32,6 +32,10 @@ const gchar * ts_get_success_msg (v
void ts_interpret_stdin (void);
+/* modified by Lukasz Czerwinski (lc277...@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.0 +0200
+++ gimp/plug-ins/script-fu/script-fu-scripts.c 2010-08-25
16:58:31.0 +0200
@@ -589,6 +589,7 @@ script_fu_run_command (const gchar *com
return success;
}
+/* modified by Lukasz Czerwinski (lc277...@students.mimuw.edu.pl) */
static void
script_fu_load_script (const GimpDatafileData *file_data,
gpointeruser_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.0 +0200
+++ gimp/plug-ins/script-fu/tinyscheme/scheme.c 2010-08-25
17:08:44.0 +0200
@@ -125,6 +125,21 @@ static int utf8_stricmp(const char *s1,
return result;
}
+/* modified by Lukasz Czerwinski (lc277...@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) <= (b) ? (a) : (b))
#if USE_STRLWR
@@ -898,15 +913,18 @@ static INLINE pointer oblist_find_by_nam
int location;
pointer x;
char *s;
+ gchar *name_case_folded = g_utf8_casefold(name, -1);
location = hash_fn(name, ivalue_unchecked(sc->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 Readin
