[Lldb-commits] [PATCH] D49612: Use LLVM's new ItaniumPartialDemangler in LLDB

[Erik Pilkington via Phabricator via lldb-commits]

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