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

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

sgraenitz added a comment.

Well when repeating this test, the values are not always that far apart from each other, but on average the old USE_BUILTIN_DEMANGLER path the slower one. Maybe FastDemangle is still faster than IPD in success case, but the overhead from the fallback cases is adding up. (The USE_BUILTIN_DEMANGLER code path is also more noisy in terms of performance, probably same issue here.)



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


https://reviews.llvm.org/D49612