On 06/07/2009 02:41 PM, Andrea Taverna wrote:
> Hi,
>
> I made a tiny change to string-related code in src/utli/string.c and
> /src/util/conv.c, following Pasky's TODO comments regarding
> optimization. Below there's the related diff output.
>
> I feel a bit embarrassed for asking. Since this is the first code I try
> to submit for an open source project, I don't have much of a clue on how
> to do it properly. What should I do next?
Thank you for your contribution. Here are two general suggestions:
1. Send your patch as an attachment to avoid mangling of white space.
2. Please follow the formatting style used in the project, which is
outlined in doc/hacking.txt. In particular, put declarations only at
the start of a block, and indent and place braces as is done in the
existing code.
>
> regards,
>
> Andrea
>
> ==============DIFF OUTPUT ========================
> diff -ru old/elinks/src/util/conv.c elinks/src/util/conv.c
> --- old/elinks/src/util/conv.c 2009-06-07 14:44:19.000000000 +0200
> +++ elinks/src/util/conv.c 2009-06-07 20:37:47.000000000 +0200
> @@ -340,17 +340,34 @@
> return string;
> }
>
> -/* TODO Optimize later --pasky */
> +
> struct string *
> add_quoted_to_string(struct string *string, const unsigned char *src,
> int len)
> {
> - for (; len; len--, src++) {
> - if (isquote(*src) || *src == '\\')
> - add_char_to_string(string, '\\');
> - add_char_to_string(string, *src);
add_char_to_string asserts that neither pointer is NULL and that the
second string is not empty. It also calls check_string_magic.
add_quoted_to_string should probably perform these checks once.
> - }
>
> - return string;
> + if (len > 0)
> + {
Place the open brace on the same line as the if clause, and indent using
tabs.
> + const unsigned char *c;
> + int i;
> + unsigned int old_length = string->length;
> +
> +
Avoid trailing white space (you have a tab on the second blank line there).
> + for(i = len, c = src; i; i--, c++){
> + if (isquote(*c) || *c =='\\')
> + string->length++;
> + string->length++;
> + };
> +
> + realloc_string(string, string->length);
> + enforce_null_terminated(*string);
You forgot to include the definition of enforce_null_terminated in your
patch. (I do stuff like that all the time.)
> +
> + for (i = old_length, c = src ; i < string->length; i++, c++) {
> + if (isquote(*src) || *c == '\\')
> + string->source[i++] = '\\';
> + string->source[i]= (*c);
Please put spaces around the '=', and delete the extra parentheses
around '*c'.
> + }
> + };
> + return string;
> }
>
> struct string *
It looks like the only code that uses add_quoted_to_string is the
options saving code and the options manager (via the write() callbacks
in the options_types structure), and saving with config.saving_style = 1
is already instantaneous for me, so it isn't all that time critical.
Aside from that, have you profiled to see whether there is a measurable
increase in performance? Micro-optimisations don't always have the
expected results. I would guess that trading a bunch of superfluous
NUL-byte assignments, calls to check_string_magic, and re-allocations
for an extra loop will improve performance, but I'm curious as to
whether it is a substantial improvement. It might be a better trade-off
to leave this relatively seldom used routine in the old, smaller and
slightly more comprehensible form.
> diff -ru old/elinks/src/util/string.c elinks/src/util/string.c
> --- old/elinks/src/util/string.c 2009-06-07 14:44:19.000000000 +0200
> +++ elinks/src/util/string.c 2009-06-07 20:39:20.000000000 +0200
> @@ -294,9 +294,6 @@
>
> /* The new string utilities: */
>
> -/* TODO Currently most of the functions use add_bytes_to_string() as a
> backend
> - * instead we should optimize each function. */
> -
> NONSTATIC_INLINE struct string *
> #ifdef DEBUG_MEMLEAK
> init_string__(const unsigned char *file, int line, struct string *string)
> @@ -351,7 +348,16 @@
>
> if (!*source) return string;
>
> - return add_bytes_to_string(string, source, strlen(source));
> + size_t newlength = string->length + strlen(source);
> +
> + realloc_string(string, newlength);
> +
> + memcpy(&(string->source[string->length]), source, newlength -
> string->length);
> +
> + string->length = newlength;
> + string->source[string->length] = '\0';
> +
> + return string;
> }
>
> /** @relates string */
> @@ -385,7 +391,16 @@
>
> if (!from->length) return string; /* optimization only */
>
> - return add_bytes_to_string(string, from->source, from->length);
> + size_t newlength = string->length + from->length;
> +
> + realloc_string(string, newlength);
> +
> + memcpy(&(string->source[string->length]), from->source, newlength -
> string->length);
> +
> + string->length = newlength;
> + string->source[string->length] = '\0';
> +
> + return string;
> }
>
> /** @relates string */
If the compiler inlines add_bytes_to_string, I don't see any difference
between the old add_to_string and add_string_to_string and the new
(besides debugging code, which can be eliminated by configuring with
CONFIG_DEBUG undefined and CONFIG_FASTMEM defined). Can you explain or
use a profiler to show any difference in running time? I don't know what
pasky had in mind with his comment about optimising this code.
Thanks for taking the time to write these patches. I hope that my
comments are helpful.
Best regards,
--
Miciah Masters <[email protected]> / <[email protected]>
_______________________________________________
elinks-dev mailing list
[email protected]
http://linuxfromscratch.org/mailman/listinfo/elinks-dev
