[Lldb-commits] [PATCH] D54074: CPlusPlusLanguage: Use new demangler API to implement type substitution
erik.pilkington added a comment. Looks good to me too, thanks! https://reviews.llvm.org/D54074 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50473: [Demangle] Add another test for ItaniumPartialDemangler
erik.pilkington accepted this revision. erik.pilkington added a comment. This revision is now accepted and ready to land. LGTM, thanks for doing this! Comment at: unittests/Demangle/PartialDemangleTest.cpp:188 +size_t N = OriginalSize; +char *Res = D.getFunctionName(Buf, ); +EXPECT_EQ(nullptr, Res); Does LLDB actually pass in a N that is less than the length of Buf? Its not wrong to do that per se, realloc will sort it out, but it seems kinda strange. Repository: rL LLVM https://reviews.llvm.org/D50473 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()
erik.pilkington added inline comments. Comment at: source/Symbol/Symtab.cpp:274 + case ItaniumPartialDemangler: +m_IPD_buf = m_IPD->getFunctionBaseName(m_IPD_buf, _IPD_size); +return m_IPD_buf; sgraenitz wrote: > @erik.pilkington Is it acceptable/good practice to pass `(nullptr, 0)` here? > At the moment this safes some lines of initialization checks for `m_IPD_buf` > and `m_IPD_size`. Sure, thats fine! Those parameters act the same way as `buf` and `size` in __cxa_demangle. `getFunctionBaseName` will return nullptr if the mangled name isn't a function. Is it a precondition of this function that m_IPD stores a function? If not, it looks like you'll leak the buffer. https://reviews.llvm.org/D49990 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49612: Use LLVM's new ItaniumPartialDemangler in LLDB
erik.pilkington 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: > 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. https://reviews.llvm.org/D49612 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49612: Use LLVM's new ItaniumPartialDemangler in LLDB
erik.pilkington added a comment. In https://reviews.llvm.org/D49612#1171550, @sgraenitz wrote: > Quick local performance check doing `target create clang` in current review > version vs. master HEAD version (both loading the exact same release build of > clang) looks promising. It's faster already now, so I would remove the option > for the builtin demangling. > > review version = LLDB_USE_LLVM_DEMANGLER: > TargetList::CreateTarget (file = 'clang', arch = 'x86_64') = 0.762155337 sec > > master HEAD version = LLDB_USE_BUILTIN_DEMANGLER: > TargetList::CreateTarget (file = 'clang', arch = 'x86_64') = 1.010040359 sec > Oh, nice! I was expecting FastDemangle to still beat the partial demangler. If FastDemangle is now slower than maybe it should be removed (or at least renamed!). 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: > > 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? https://reviews.llvm.org/D49612 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits