sgraenitz marked 2 inline comments as done.
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);
----------------
labath wrote:
> 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.
Yes. I will reach out to the list for discussion of options, when I am done
with a few experiments in that direction.
https://reviews.llvm.org/D49612
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
