Page MenuHomePhabricator

[clangd] Increase stack size of the new threads on macOS
AcceptedPublic

Authored by Dmitry.Kozhevnikov on Aug 20 2018, 1:10 PM.

Details

Summary

By default it's 512K, which is way to small for clang parser to run on. There is no way to do it via platform-independent API, so it's implemented via pthreads directly in clangd/Threading.cpp.

Diff Detail

Event Timeline

  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 wonder if we should expose the stack size as an option to clangd?
clangd/Threading.cpp
57 ↗(On Diff #161549)

Maybe add a parameter for the stack size to this function? (And document it's only taken into account when pthread-based implementation is used)

57 ↗(On Diff #161549)

Maybe accept a llvm::unique_function<void()> instead of UserFn and Data? This split is clearly implementation detail of pthreads-based solution. std::thread does not need it.

  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.

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 alien for me.

  1. I wonder if we should expose the stack size as an option to clangd?

Do you expect it would be ever used?

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 alien for me.

Yeah, we should probably do both Unix and Windows there. My guess is that the Windows implementation shouldn't be too hard, since most of the code is already there.
In any way, it's fine to start with a Unix-only version and a stub in Windows and figuring out the implementation during the review. Happy to look into Windows myself, it shouldn't be too hard :-)

  1. I wonder if we should expose the stack size as an option to clangd?

Do you expect it would be ever used?

Not frequently, but I don't see how we can figure out good defaults there. It's one of those things where the guideline is "as small as possible, as long as it does not crash clangd".
An option could provide a simple workaround for cases when clangd is actually crashing.

jfb added a subscriber: jfb.Aug 21 2018, 9:40 AM

Isn't this duplicating code in lib/Support/Unix/Threading.inc with a different implementation?

arphaman added inline comments.Aug 21 2018, 3:23 PM
clangd/Threading.cpp
76 ↗(On Diff #161549)

please use clang::DesiredStackSize instead.

In 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 some dance with the passed function lifetime is required, while llvm_execute_on_thread just uses a local variable since it blocks till the thread is terminated.

Still, I see that it's desirable to have the new code in Support/Threading, I'll move it there.

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.

clangd/Threading.cpp
76 ↗(On Diff #161549)

Oh, cool! TIL we have this. Thanks for mentioning it.

I've moved the platform-specific implementation to LLVM/Support/Threading and created https://reviews.llvm.org/D51103.

sammccall accepted this revision.Aug 23 2018, 1:59 AM

This part looks good (I think everything controversial got moved into the other patch). Thanks!

clangd/Threading.cpp
75 ↗(On Diff #161954)

nit: maybe call these CallAction and CallCleanupTask or something to make the parallels completely explicit. Up to you

This revision is now accepted and ready to land.Aug 23 2018, 1:59 AM
ilya-biryukov accepted this revision.Aug 23 2018, 6:05 AM

LGTM. Many thanks!
See the NIT about avoiding the helper class too.

clangd/Threading.cpp
88 ↗(On Diff #161954)

NIT: we usually do the following to avoid writing those classes (not great, but arguably a bit less boilerplate):

// 'Bind' is in 'Function.h'
Bind([](decltype(ThreadFunc) ThreadFunc, decltype(CleanupTask), std::string ThreadName) {
// ... code
},
std::move(Action), std::move(CelanupTask), std::move(ThreadName));

@Dmitry.Kozhevnikov, do you need us to land the patch?
Any last changes you'd like to make?

@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, when both are accepted, I’ll need someone to commit it, yes :)

ilya-biryukov added a comment.EditedAug 24 2018, 2:53 AM

@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, when both are accepted, I’ll need someone to commit it, yes :)

Ping us when the prerequisites are ready and this should be merged.
And make sure to request commit access for yourself after someone lands a few patches for you ;-)

arphaman accepted this revision.Aug 24 2018, 3:10 PM

Is there any good reason not to land this? Clangd is crashing for me on macOS with stack overflow in the worker threads.

Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 1:16 PM
Herald added a subscriber: dexonsmith. · View Herald Transcript

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.

We should definitely land this.

@Dmitry.Kozhevnikov, you don't have commit access, right? Should we land these two revisions for you?

D61724 refactors BackgroundIndexer to use AsyncTaskRunner.
So by the time this lands, it should cover all places where threads are created in clangd.

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 done for the Windows part, also I really don’t like how it’ interacting with threading disabled (that’s something I’ve realized after submitting the review) - so I’ve abandoned that, sorry. I could make an another take next week - or should we wait for D61724?

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 done for the Windows part, also I really don’t like how it’ interacting with threading disabled (that’s something I’ve realized after submitting the review) - so I’ve abandoned that, sorry. I could make an another take next week - or should we wait for D61724?

D61724 has landed.

LLVM_ENABLE_THREADS=Off is now "officially" unsupported in clangd (rL360115), so if it's not sensible to have this in llvm/Support due to LLVM_ENABLE_THREADS then we can put it in clangd instead.

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.

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.

Yes, that’s probably a better way. I’ll be back at it in a few days.

Dmitry.Kozhevnikov changed the repository for this revision from rCTE Clang Tools Extra to rG LLVM Github Monorepo.

moved to the monorepo