From: sami wagiaalla <swagiaal@redhat.com>
To: tromey@redhat.com
Cc: gdb-patches@sourceware.org
Subject: Re: [patch] Change cplus_specific to an alocated struct
Date: Mon, 14 Jun 2010 19:29:00 -0000 [thread overview]
Message-ID: <4C167FCA.7030300@redhat.com> (raw)
In-Reply-To: <m34ohdlap4.fsf@fleche.redhat.com>
> Sami> char *
> Sami> ada_decode_symbol (const struct general_symbol_info *gsymbol)
> Sami> {
> Sami> - char **resultp =
> Sami> - (char **)&gsymbol->language_specific.cplus_specific.demangled_name;
> Sami> - if (*resultp == NULL)
> Sami> + char *result = symbol_get_cplus_demangled_name (gsymbol);
>
> I don't think this change is correct -- but FWIW I don't think the
> existing code is correct, either.
>
> The code is written this way because this function caches the demangled
> name in the cplus_specific struct. Your change removes this caching.
>
Totally missed this :)
Badness of the original code aside my patch can be corrected by using
the setter function.
> All this reminds me -- you should look at bcache utilization and memory
> use before and after your changes to make sure we aren't hitting a
> memory use regression here. There is some command that will show you
> bcache statistics, though I forget what it is offhand.
>
I used "maint print statistics" there are no changes in the "Total
memory used for *" fields. But that differ seem to differ even between
runs of the same version. Here are the results of a couple of diffs:
$ diff before after
790c790
< Hash table population: 43%
---
> Hash table population: 44%
793c793
< Maximum hash chain length: 5
---
> Maximum hash chain length: 6
[swagiaal@toner build]$ diff before after
237c237
< Hash table population: 45%
---
> Hash table population: 44%
632c632
< Hash table population: 55%
---
> Hash table population: 53%
793c793
< Maximum hash chain length: 4
---
> Maximum hash chain length: 6
> Sami> +char *
> Sami> +symbol_get_cplus_demangled_name (const struct general_symbol_info *gsymbol)
> Sami> +{
> Sami> + if (gsymbol->language_specific.cplus_specific != NULL)
> Sami> + return gsymbol->language_specific.cplus_specific->demangled_name;
> Sami> +
> Sami> + return NULL;
> Sami> +}
>
> Perhaps we should have different types of structs in the
> language_specific union, and have functions like this examine the
> language field.
>
> It seems to me that soon we're going to want to add a bunch of
> C++-specific fields, and we don't want to unnecessarily penalize the
> other languages with our baggage.
>
> If you were planning that for a follow-on patch, that is fine -- but
> also the sort of thing that it is handy to note in your submissions.
>
I wasn't really planing one :D, but what do you think of this:
We leave the current struct as is and rename cplus_specific to
mangled_lang_specific (or just mangled_lang). And the the union we add a
cplus_specific that managed as things are in this patch, and is actually
cplus_specific ?
> Sami> +/* Initialize the cplus_specific structure. 'cplus_specific' is intended to
> Sami> + be allocated lazily. So this should only be called if the structure is
> Sami> + needed. */
> Sami> +static void
> Sami> +symbol_init_cplus_specific (struct general_symbol_info *gsymbol,
> Sami> + struct objfile *objfile)
>
> Initializing lazily is usually good, but it breaks the bcache.
> However perhaps this can be fixed by having the partial symbol code
> force the issue.
>
I think that is just a wrong use of lazy here. I meant to say initialize
it /if/ it is going to be used rather than when...
symbol_init_cplus_specific is called from symbol_set_names where the
bcache is updated.
next prev parent reply other threads:[~2010-06-14 19:29 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-05-26 16:05 sami wagiaalla
2010-05-26 16:55 ` sami wagiaalla
2010-06-08 17:02 ` Tom Tromey
2010-06-14 19:29 ` sami wagiaalla [this message]
2010-06-15 22:56 ` Tom Tromey
2010-07-12 18:03 ` [patch 1/3] " sami wagiaalla
2010-07-13 17:16 ` Tom Tromey
2010-07-16 14:15 ` sami wagiaalla
2010-07-16 15:24 ` Tom Tromey
2010-07-12 18:06 ` [patch 2/3] " sami wagiaalla
2010-07-13 17:24 ` Tom Tromey
2010-07-16 14:15 ` sami wagiaalla
2010-07-12 18:08 ` [patch 3/3] " sami wagiaalla
2010-07-13 17:38 ` Tom Tromey
2010-07-16 14:15 ` sami wagiaalla
2010-07-16 15:29 ` Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C167FCA.7030300@redhat.com \
--to=swagiaal@redhat.com \
--cc=gdb-patches@sourceware.org \
--cc=tromey@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).