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

[Stefan Gränitz via Phabricator via lldb-commits]

sgraenitz added inline comments.


================
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:
> > 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?


https://reviews.llvm.org/D49612