Re: [patchv2 2/2] Accelerate lookup_symbol_aux_objfile 14.5x [Re: [patch 0/2] Accelerate symbol lookups 15x]

Jan Kratochvil
2014-10-27T08:54:45Z

On Mon, 27 Oct 2014 06:54:15 +0100, Doug Evans wrote:
> I'd be grateful if you could replace your 1/2 and 2/2 with this experiment
> and see what numbers you get. It's good to have data.

Benchmark on non-trivial application with 'p <tab><tab>':
Command execution time: 0.091000 (cpu), 0.092709 (wall) --- Doug's fix
Command execution time: 4.215000 (cpu), 4.241466 (wall) --- both fixes with new [patch 2/2]
Command execution time: 7.373000 (cpu), 7.395095 (wall) --- both fixes
Command execution time: 13.572000 (cpu), 13.592689 (wall) --- just lookup_symbol_aux_objfile fix
Command execution time: 113.036000 (cpu), 113.067995 (wall) --- FSF GDB HEAD

That is 113.067995/0.092709 = 1219x improvement.

> Alas, the experiment is just that because gdb only looks up
> some symbols from expanded symtabs and not partial symtabs/gdb_index,
> because neither partial syms nor the index record all symbols,
> and thus there are several testsuite regressions.
> We would have to fix this.

OK, so running a regression testsuite with your patch is pointless now.

> However, for basic symbol lookup, only searching the index, and never
> searching already expanded symtabs, makes sense: the index knows
> where the symbol lives, so why search anywhere else?

What about inlined function instances? Are they in .gdb_index? And if they are
we need all their instances while .gdb_index always points to only one
instance. I did not check/test it, just an idea now.

> And in the null case, which is what is killing performance in your example,
> we certainly want to go to the index first, not second.

I was looking if Tom Tromey justified why
quick_symbol_functions::lookup_symbol returns NULL on already expanded symtabs
- this was introduced by:
Subject: [0/4] RFC: refactor partial symbol tables
Message-ID: <m38wbyc31o.fsf@fleche.redhat.com>
But I haven't found anything, probably just to make the implementation
safer/easier.

> So the question is, what to do in the meantime.
>
> I'm ok with your 2/2 patch (with the changes I've requested) since I think
> it's reasonable regardless of anything else.
> [btw, I've submitted a patch to move lookup_block_symbol to block.c:
> https://sourceware.org/ml/gdb-patches/2014-10/msg00720.html]

OK, I will rebase the patch 2/2 after it gets checked in.

> Your 1/2 patch (dictionary hash caching) still gives me pause.
> I didn't have time to collect more timing data this weekend.
> I might be ok with it going in provided it can be removed without
> effort if/when the above improvements are applied.

The improvements above IIUC apply only for objfiles with .gdb_index.
That patch 1/2 applied even for non-.gdb_index objfiles.

> Before this patch with plain FSF GDB I get 7.5 seconds for "p/r var".
> With this patch it's 0.005.

It matches my benchmark above.

Thanks,
Jan