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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
