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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

The patch makes sense to me. It's nice to get a performance boost, while reducing the number of demanglers at the same time.



================
Comment at: source/Core/Mangled.cpp:310
+#elif defined(LLDB_USE_LLVM_DEMANGLER)
+        llvm::ItaniumPartialDemangler IPD;
+        bool demangle_err = IPD.partialDemangle(mangled_name);
----------------
erik.pilkington wrote:
> sgraenitz wrote:
> > erik.pilkington wrote:
> > > 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?
> > Yes if  that will be a bit more work, but also a possibility. I did a small experiment making the IPD in line 288 `static`, to reuse a single instance. That didn't affect runtimes much. I tried it several times and got the same results again, but have no explanation.
> > 
> > You would expect a speedup from this right? Is there maybe cleanup work that eats up time when reusing an instance? Maybe I have to revisit that.
> Weird, I would have expected a speedup for making it static. My only guess is that `malloc` is just quickly handing back the buffer it used for the last IPD or something. I think the cleanup work IPD does should be about the same as the cost of construction.
If reusing the same IPD object can bring significant benefit, I think the right approach would be to change/extend/add the API (not in this patch) to make it possible to use it naturally. There aren't many places that do batch demangling of a large amount of symbols (`Symtab::InitNameIndexes` is one I recall), so it shouldn't be too hard to modify them to do something smarter.


https://reviews.llvm.org/D49612