[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

This revision was automatically updated to reflect the committed changes. Closed by commit rG69a39dc1f0d0: [clangd] Increase stack size of the new threads on macOS (authored by sammccall). Changed prior to commit: https://reviews.llvm.org/D50993?vs=200432=242319#toc Repository: rG LLVM

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

sammccall added a comment. Herald added a subscriber: usaxena95. Sorry we let this last piece get stuck in limbo, of course I was reminded of it when someone else hit this problem. Thanks for working on this @Dmitry.Kozhevnikov, I'll update for c++14 and (finally) land it now. Repository:

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov updated this revision to Diff 200432. Dmitry.Kozhevnikov changed the repository for this revision from rCTE Clang Tools Extra to rG LLVM Github Monorepo. Dmitry.Kozhevnikov added a comment. moved to the monorepo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov added a comment. In D50993#1496946 , @ilya-biryukov wrote: > I'd still put it into LLVM to avoid platform-specific code in clangd. > Maybe `std::abort()` in the added `...Async` function if threads are > disabled? It's a bit

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. I'd still put it into LLVM to avoid platform-specific code in clangd. Maybe `std::abort()` in the added `...Async` function if threads are disabled? It's a bit unusual, but would allow keeping this function where it belongs. Repository: rCTE Clang Tools Extra

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

sammccall added a comment. In D50993#1496638 , @Dmitry.Kozhevnikov wrote: > In D50993#1496250 , @ilya-biryukov > wrote: > > > We should definitely land this. > > > > @Dmitry.Kozhevnikov, you don't have commit

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov added a comment. In D50993#1496250 , @ilya-biryukov wrote: > We should definitely land this. > > @Dmitry.Kozhevnikov, you don't have commit access, right? Should we land > these two revisions for you? The depending review was never

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. D61724 refactors `BackgroundIndexer` to use `AsyncTaskRunner`. So by the time this lands, it should cover all places where threads are created in clangd. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. We should definitely land this. @Dmitry.Kozhevnikov, you don't have commit access, right? Should we land these two revisions for you? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50993/new/

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

umanwizard added a comment. By the way, index/Background.{cpp,h} also need to be changed to use the new API. I made that change locally and clangd with full project indexing is now working for me on macOS. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

umanwizard added a comment. Herald added a subscriber: dexonsmith. Herald added a project: clang. Is there any good reason not to land this? Clangd is crashing for me on macOS with stack overflow in the worker threads. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov added a comment. In https://reviews.llvm.org/D50993#1212130, @ilya-biryukov wrote: > @Dmitry.Kozhevnikov, do you need us to land the patch? > Any last changes you'd like to make? It’s blocked by the llvm review, which has some valid comments I’ll address today. After that,

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. @Dmitry.Kozhevnikov, do you need us to land the patch? Any last changes you'd like to make? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50993 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov accepted this revision. ilya-biryukov added a comment. LGTM. Many thanks! See the NIT about avoiding the helper class too. Comment at: clangd/Threading.cpp:88 + llvm::llvm_execute_on_thread_async( + Callable{Name.str(), std::move(Action),

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. This part looks good (I think everything controversial got moved into the other patch). Thanks! Comment at: clangd/Threading.cpp:75 +std::string ThreadName; +

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov updated this revision to Diff 161954. Dmitry.Kozhevnikov added a comment. I've moved the platform-specific implementation to LLVM/Support/Threading and created https://reviews.llvm.org/D51103. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50993 Files:

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. WRT to the configuration argument, using stack size suggested by @arphaman seems like a better option. So let's not add any extra options to clangd. Comment at: clangd/Threading.cpp:76 + + if (::pthread_attr_setstacksize(, 8 * 1024 * 1024) !=

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov added a comment. In https://reviews.llvm.org/D50993#1207757, @jfb wrote: > Isn't this duplicating code in lib/Support/Unix/Threading.inc with a > different implementation? It's definitely duplicating how the thread is created, however, here it's a bit more complicated as

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

arphaman added inline comments. Comment at: clangd/Threading.cpp:76 + + if (::pthread_attr_setstacksize(, 8 * 1024 * 1024) != 0) +return; please use clang::DesiredStackSize instead. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50993

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

jfb added a comment. Isn't this duplicating code in lib/Support/Unix/Threading.inc with a different implementation? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50993 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. In https://reviews.llvm.org/D50993#1207464, @Dmitry.Kozhevnikov wrote: > Do you think it would be OK to have pthreads-only version there, with > `std::thread` fallback on Windows? I'd like to avoid writing a Windows-only > implementation myself, as it's a bit

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov added a comment. In https://reviews.llvm.org/D50993#1207357, @ilya-biryukov wrote: > 1. Can we put the helper that executes the tasks asynchronously with a stack > size into llvm's threading library (i.e. somewhere close to > `llvm::execute_on_thread`?) All platform-specific

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

ilya-biryukov added a comment. 1. Can we put the helper that executes the tasks asynchronously with a stack size into llvm's threading library (i.e. somewhere close to `llvm::execute_on_thread`?) All platform-specific code is already there, we can reuse most of the implementation too. 2. I

[PATCH] D50993: [clangd] Increase stack size of the new threads on macOS

Dmitry.Kozhevnikov created this revision. Dmitry.Kozhevnikov added reviewers: ilya-biryukov, sammccall. Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, ioeric. By default it's 512K, which is way to small for clang parser to run on. There is no way to do it via