Re: [PATCH] ld: pe: Improve performance of object file exclude symbol directives

[Nick Clifton <nickc@redhat.com>]

Hi Martin,

> Store the list of excluded symbols in a sorted list, speeding up
> checking for duplicates when inserting new entries.

An excellent idea.

>     /* From EXCLUDE_SYMBOLS or embedded directives. */
> +  int num_exclude_symbols;

Presumably there can never be a negative number of excluded symbols,
so using "unsigned int" here might be more appropriate.


> +  if (fdef->exclude_symbols)

Given that the for() loop to follow checks fdef->num_exclude_symbols,
this if() statement is redundant.

> +      for (i = 0; i < fdef->num_exclude_symbols; i++)
> +	   free (fdef->exclude_symbols[i].symbol_name);
> +      free (fdef->exclude_symbols);

Calling free(NULL) is allowed, so the if() statement is still not needed.

> +/* Search the position of the identical element, or returns the position
> +   of the next higher element. If last valid element is smaller, then MAX
> +   is returned.  */

The comment should explain the use of the IS_IDENT parameter.

It should probably also note that MAX is expected to be the same as the number
of entries in the B array.

> +static int
> +find_exclude_in_list (def_file_exclude_symbol *b, int max,
> +		      const char *name, int *is_ident)

Since we are dealing with array indicies here, I would suggest that the function
should return an unsigned int.  Likewise the MAX parameter should be unsigned.
Plus is_ident should be a boolean pointer, not an integer pointer.


> +static def_file_exclude_symbol *
> +def_file_add_exclude_symbol (def_file *fdef, const char *name, int *is_dup)

I think that IS_DUP should be a "boolean *" not a "int *".


> +  int max_exclude_symbols = ROUND_UP(fdef->num_exclude_symbols, 32);

Magic numbers are bad.  Please replace 32 with a defined constant.

> +  if (fdef->num_exclude_symbols >= max_exclude_symbols)

This is sneaky and not very intuitive.  I would prefer it if you had
two fields in the fdef structure, one for the number of allocated
slots in the exclude_symbols array and one for the number of used slots.

Then testing for when the array needs to be extended would be a case
of comparing these two fields.  Using two fields might take up more
space in the structure, but it sure makes a lot more sense to me.


> +      max_exclude_symbols = ROUND_UP(fdef->num_exclude_symbols + 1, 32);

Given that the point of this patch is to improve performance when there
are a large number of excluded symbols, incrementing the array by 32 slots
at a time seems counter intuitive.  I would suggest a bigger number, eg 1024
or 10240.


> +  e = fdef->exclude_symbols + pos;
> +  if (pos != fdef->num_exclude_symbols)
> +    memmove (&e[1], e, (sizeof (def_file_exclude_symbol) * (fdef->num_exclude_symbols - pos)));

What the ?  OK, so what is going on here ?  At the very least I think that
you are going to need to add a comment to this piece of code.


Cheers
   Nick

Cheers
   Nick