Re: Inheritance of gfc_symbol / gfc_component

[Janne Blomqvist <blomqvist dot janne at gmail dot com>]

On Thu, Aug 16, 2012 at 5:17 PM, Tobias SchlÃter
<tobias.schlueter@physik.uni-muenchen.de> wrote:
> On 2012-08-16 15:48, Tobias Burnus wrote:
>>
>> On 08/16/2012 02:59 PM, Tobias SchlÃter wrote:
>>>
>>> A place where C++ inheritance is a trivial improvement is the
>>> red-black tree used for storing various objects (gfc_symtree,
>>> gfc_gsymbol, gfc_st_label, I think). This is currently implemented
>>> with macro-based inheritance. It is trivial to replace the macro with
>>> C++ inheritance, but if one touches this code, it might make more
>>> sense to do a real job instead and to convert the symbol-keeping code
>>> to the compiler's hash-table implementation, which as a benefit should
>>> make the compiler faster by get rid of lots of string comparisons.
>>
>>
>> Well, most string comparisons in gfortran are of the type ..->name ==
>> ...->name, which is fast. The reason is that those are obtained via
>> gfc_get_string, which in turn calls:
>> ident = get_identifier (temp_name);
>> return IDENTIFIER_POINTER (ident);
>>
>> Thus, same name == same pointer. (I think in some cases we do call
>> strcmp even when a normal comparison would do.)
>
>
> I beg to differ ;-)
>
> static int
> compare_symtree (void *_st1, void *_st2)
> {
>   gfc_symtree *st1, *st2;
>
>   st1 = (gfc_symtree *) _st1;
>   st2 = (gfc_symtree *) _st2;
>
>   return strcmp (st1->name, st2->name);
> }
>
> which is then used when creating a new symtree:
>   gfc_insert_bbt (root, st, compare_symtree);
>
> I remember this because I tried to replace this with pointer compares when I
> migrated the gfc_get_string machinery from g95.  This failed for reasons
> that I don't distinctly remember.

I recall that at some point I was profiling the compiler when
investigating why our module handling is so awfully slow, and strcmp
did show up quite high in the profile. IIRC when loading a module in
some situations we do a brute-force search to find symbols with the
same name; for some reason the "normal" tree search couldn't be used
as it was keyed on another field, or something like that.

>  One reason may be that alphabetical
> ordering of symbols couldn't be maintained that way, which is a problem, as
> we use this ordering to ensure that modules are human-readable and only get
> rewritten when necessary.

Currently the order is determined by the address ordering, which is
not guaranteed to be repeatable. See PR 51727.

> BTW a hashtable would also run into this problem.

Indeed. One could add an "id" field which is allocated sequentially
when parsing the  source file, instead of relying on the address
ordering. If one would go with an inheritance based design, this field
would be a good candidate to include in the superclass; that being
said, I haven't looked at Mikael's patch so I can't comment further on
it.


-- 
Janne Blomqvist