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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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