erik.pilkington added a comment.
In https://reviews.llvm.org/D49612#1171550, @sgraenitz wrote:
> Quick local performance check doing `target create clang` in current review
> version vs. master HEAD version (both loading the exact same release build of
> clang) looks promising. It's faster already now, so I would remove the option
> for the builtin demangling.
>
> review version = LLDB_USE_LLVM_DEMANGLER:
> TargetList::CreateTarget (file = 'clang', arch = 'x86_64') = 0.762155337 sec
>
> master HEAD version = LLDB_USE_BUILTIN_DEMANGLER:
> TargetList::CreateTarget (file = 'clang', arch = 'x86_64') = 1.010040359 sec
>
Oh, nice! I was expecting FastDemangle to still beat the partial demangler. If
FastDemangle is now slower than maybe it should be removed (or at least
renamed!).
================
Comment at: source/Core/Mangled.cpp:310
+#elif defined(LLDB_USE_LLVM_DEMANGLER)
+ llvm::ItaniumPartialDemangler IPD;
+ bool demangle_err = IPD.partialDemangle(mangled_name);
----------------
sgraenitz wrote:
> sgraenitz wrote:
> > sgraenitz wrote:
> > > erik.pilkington wrote:
> > > > I think this is going to really tank performance:
> > > > ItaniumPartialDemangler dynamically allocates a pretty big buffer on
> > > > construction that it uses to store the AST (and reuse for subsequent
> > > > calls to partialDemangle). Is there somewhere that you can put IPD it
> > > > so that it stays alive between demangles?
> > > >
> > > > An alternative, if its more convenient, would be to just put the buffer
> > > > inline into ItaniumPartialDemangler, and `= delete` the move operations.
> > > Thanks for the remark, I didn't dig deep enough to see what's going on in
> > > the `BumpPointerAllocator` class. I guess there is a reason for having
> > > `ASTAllocator` in the `Db` struct as an instance and thus allocating
> > > upfront, instead of having a pointer there and postponing the
> > > instantiation to `Db::reset()`?
> > >
> > > I am not familiar enough with the code yet to know how it will look
> > > exactly, but having a heavy demangler in every `Mangled` per se sounds
> > > unreasonable. There's just too many of them that don't require demangling
> > > at all. For each successfully initiated `partialDemangle()` I will need
> > > to keep one of course.
> > >
> > > I will have a closer look on Monday. So far thanks for mentioning that!
> > Well, right the pointer to `BumpPointerAllocator` won't solve anything. Ok
> > will have a look.
> > ItaniumPartialDemangler dynamically allocates a pretty big buffer on
> > construction
>
> I think in the end each `Mangled` instance will have a pointer to IPD field
> for lazy access to rich demangling info. This piece of code may become
> something like:
> ```
> m_IPD = new ItaniumPartialDemangler();
> if (bool err = m_IPD->partialDemangle(mangled_name)) {
> delete m_IPD; m_IPD = nullptr;
> }
> ```
>
> In order to avoid unnecessary instantiations, we can consider to filter
> symbols upfront that we know can't be demangled. E.g. we could duplicate the
> simple checks from `Db::parse()` before creating a `ItaniumPartialDemangler`
> instance.
>
> Even the simple switch with the above code in place shows performance
> improvements. So for now I would like to leave it this way and review the
> issue after having the bigger patch, which will actually make use of the rich
> demangle info.
>
> What do you think?
Sure, if this is a performance win then I can't think of any reason not to do
it.
In the future though, I don't think that having copies of IPD in each Mangled
is a good idea, even if they are lazily initialized. The partially demangled
state held in IPD is significantly larger than the demangled string, so I think
it would lead to a lot more memory usage. Do you think it is possible to have
just one instance of IPD that you could use to demangle all the symbols to
their chopped-up state, and just store that instead?
https://reviews.llvm.org/D49612
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
