[REVERTED PATCH] symbol lookup cache

[REVERTED PATCH] symbol lookup cache

[Doug Evans]

This never showed up in my regular testing, sigh, but
there was a problem in the symbol lookup cache.
It seems like it's easy enough to fix (see PR 17799)
but I wasn't sure how fixed the schedule is, and wasn't
sure whether there might be more such issues.

A 20x speedup on a simple "ptype std_string_var" is really nice,
[and it'll only get better the larger the app, e.g., the larger the number
of shared libraries], and I expect there'll be such examples,
but whether the patch is ready for release ... I think it is, but I've
already found one issue ...

So I reverted the symbol lookup cache patch in commit
6a3ca067521821b6c2ad9a836104d11e6dd760cb

commit 6a3ca067521821b6c2ad9a836104d11e6dd760cb
Author: Doug Evans <xdje42@gmail.com>
Date:   Sun Jan 11 15:16:26 2015 -0800

    Temporarily revert symbol lookup cache.

    clear_symtab_users calls breakpoint_re_set before
    observer_notify_new_objfile(NULL), and thus symbol lookup
    done during breakpoint_re_set will see a stale cache.

    Presumably we just need to move the call to
observer_notify_new_objfile(NULL)
    to before breakpoint_re_set, but need to check for other such issues,
    and 7.9 is scheduled to branch tomorrow.

    Reverts commits:
    b2fb95e006c29e2cbe4b30523879fe3640f906ad
    400678a494713abf8f7ea2367f213109a2c4b886
    d98b9ccbccf36563dad92f6093a93655b38bc51b
    77087adf50cedf78cc216ac6eb3b2863839d713c

    gdb/ChangeLog:

        * symtab.c (eq_symbol_entry): Use SYMBOL_SEARCH_NAME and
        symbol_matches_domain for symbol comparisons.

        * symtab.c (symbol_cache_mark_found): Improve function comment.
        Rename parameter objfile to objfile_context.
        (symbol_cache_mark_not_found): Improve function comment.

        Add symbol lookup cache.
        * NEWS: Document new options and commands.
        * symtab.c (symbol_cache_key): New static global.
        (DEFAULT_SYMBOL_CACHE_SIZE, MAX_SYMBOL_CACHE_SIZE): New macros.
        (SYMBOL_LOOKUP_FAILED): New macro.
        (symbol_cache_slot_state): New enum.
        (block_symbol_cache): New struct.
        (symbol_cache): New struct.
        (new_symbol_cache_size, symbol_cache_size): New static globals.
        (hash_symbol_entry, eq_symbol_entry): New functions.
        (symbol_cache_byte_size, resize_symbol_cache): New functions.
        (make_symbol_cache, free_symbol_cache): New functions.
        (get_symbol_cache, symbol_cache_cleanup): New function.
        (set_symbol_cache_size, set_symbol_cache_size_handler): New functions.
        (symbol_cache_lookup, symbol_cache_clear_slot): New function.
        (symbol_cache_mark_found, symbol_cache_mark_not_found): New functions.
        (symbol_cache_flush, symbol_cache_dump): New functions.
        (maintenance_print_symbol_cache): New function.
        (maintenance_flush_symbol_cache): New function.
        (symbol_cache_stats): New function.
        (maintenance_print_symbol_cache_statistics): New function.
        (symtab_new_objfile_observer): New function.
        (symtab_free_objfile_observer): New function.
        (lookup_static_symbol, lookup_global_symbol): Use symbol cache.
        (_initialize_symtab): Init symbol_cache_key.  New parameter
        maint symbol-cache-size.  New maint commands print symbol-cache,
        print symbol-cache-statistics, flush-symbol-cache.
        Install new_objfile, free_objfile observers.

    gdb/doc/ChangeLog:

        * gdb.texinfo (Symbols): Document new commands
        "maint print symbol-cache", "maint print symbol-cache-statistics",
        "maint flush-symbol-cache".  Document new option
        "maint set symbol-cache-size".

Re: [REVERTED PATCH] symbol lookup cache

[Joel Brobecker]

> A 20x speedup on a simple "ptype std_string_var" is really nice,
> [and it'll only get better the larger the app, e.g., the larger the number
> of shared libraries], and I expect there'll be such examples,
> but whether the patch is ready for release ... I think it is, but I've
> already found one issue ...

We have the option of delaying the branch, and in fact, I will for now
delay until at least tomorrow so we can discuss what we want to do.

FWIW, here is what I think: I think symbol lookup is generally critical,
and I would be extra cautious before pushing big chunks of code that late
in the process, particularly if it's "just" to improve pre-existing
performance (pardon the "just" - it is  not qualifying nor quantifying
the benefits of your patch).

That's why I would personally leave your patch out of the 7.9 branch.
But that's only based on a general impression of the patch, so, as
I said, everyone is welcome to weigh in on the decision.

-- 
Joel