On 11/7/18 11:31 AM, Sam McCall wrote:
> I'll send a patch shortly to unbreak this.
>
> Is emitting a warning a problem for you? (i.e. do you build with -Werror)
Yes "unfortunately" we use -Werror.
> I'd like to do something like #ifdef SCHED_IDLE ... #else #warning "old
> libc?" #endif
> Tha
I'll send a patch shortly to unbreak this.
Is emitting a warning a problem for you? (i.e. do you build with -Werror)
I'd like to do something like #ifdef SCHED_IDLE ... #else #warning "old
libc?" #endif
That way if this isn't actually working we'll break in linux configurations
covered by -Werror
Hi,
On 11/7/18 11:03 AM, Sam McCall wrote:
> On Wed, Nov 7, 2018 at 10:32 AM Mikael Holmén via Phabricator
> mailto:revi...@reviews.llvm.org>> wrote:
>
> uabelho added a comment.
>
> Hi,
>
> I've got a post-review comment about the use of SCHED_IDLE vs the
> needed gcc version.
On Wed, Nov 7, 2018 at 10:32 AM Mikael Holmén via Phabricator <
revi...@reviews.llvm.org> wrote:
> uabelho added a comment.
>
> Hi,
>
> I've got a post-review comment about the use of SCHED_IDLE vs the needed
> gcc version.
>
>
>
>
> Comment at: clang-tools-extra/trunk/clangd/Thre
uabelho added a comment.
Hi,
I've got a post-review comment about the use of SCHED_IDLE vs the needed gcc
version.
Comment at: clang-tools-extra/trunk/clangd/Threading.cpp:110
+ T.native_handle(),
+ Priority == ThreadPriority::Low ? SCHED_IDLE : SCHED_OTHER, &priori
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345590: [clangd] Use thread pool for background indexing.
(authored by kadircet, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D53651
Files:
c
kadircet updated this revision to Diff 171676.
kadircet marked an inline comment as done.
kadircet added a comment.
- Format the code.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/
kadircet updated this revision to Diff 171675.
kadircet marked 2 inline comments as done.
kadircet added a comment.
- Add comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/inde
kadircet added inline comments.
Comment at: clangd/Threading.cpp:101
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
ilya-biryukov wrote:
> Maybe put this helper into `llvm/Support/Threading.h`?
We talked with Sam about
sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Nice! Sorry about the back and forth with threadpool...
Comment at: clangd/index/Background.cpp:35
+ assert(ThreadPoolSize > 0 && "Thread pool size can't be zero.");
+
kadircet updated this revision to Diff 171672.
kadircet added a comment.
- Address comments.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index: clangd/index/Ba
kadircet updated this revision to Diff 171658.
kadircet added a comment.
- Delete outdated comment.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index: clangd/i
kadircet added inline comments.
Comment at: clangd/Threading.cpp:102
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
sammccall wrote:
> don't you also need to actually include it?
Turns out it was
kadircet updated this revision to Diff 171655.
kadircet marked 14 inline comments as done.
kadircet added a comment.
- Address comments && Use ThreadPool
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Backg
ilya-biryukov added inline comments.
Comment at: clangd/Threading.cpp:101
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
Maybe put this helper into `llvm/Support/Threading.h`?
Comment at: clangd/Thre
sammccall added inline comments.
Comment at: clangd/Threading.cpp:102
+void setThreadPriority(std::thread &T, ThreadPriority Priority) {
+#ifdef HAVE_PTHREAD_H
+ sched_param priority;
don't you also need to actually include it?
Comment at: cla
kadircet updated this revision to Diff 171527.
kadircet added a comment.
- Use notify_all.
- Use priorities.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/Threading.cpp
clangd/Threading.h
clangd/index/Background.cpp
clangd/index/Background.h
Index:
ilya-biryukov added a comment.
Update from the offline meeting: decided to start with `llvm::ThreadPool` for
background tasks and lower thread priorities for background tasks.
Comment at: clangd/index/Background.cpp:89
}
- QueueCV.notify_all();
+ QueueCV.notify_one();
}
kadircet added a comment.
Moving forward with priorities then.
Comment at: clangd/index/Background.cpp:89
}
- QueueCV.notify_all();
+ QueueCV.notify_one();
}
sammccall wrote:
> I always forget the details of how these work :-\
> Is it possible for the "on
sammccall added a comment.
In https://reviews.llvm.org/D53651#1275517, @ilya-biryukov wrote:
> It's fine to spend one thread spinning on background tasks, but if we're
> going to do a threadpool, we should be more careful to not hurt the
> performance of foreground tasks. To do that, we should
ilya-biryukov added a comment.
It's fine to spend one thread spinning on background tasks, but if we're going
to do a threadpool, we should be more careful to not hurt the performance of
foreground tasks. To do that, we should at least:
- share the semaphore for the number of actively running t
kadircet created this revision.
kadircet added reviewers: sammccall, ioeric.
Herald added subscribers: cfe-commits, jfb, arphaman, jkorous, MaskRay,
ilya-biryukov.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53651
Files:
clangd/index/Background.cpp
clangd/index/Background
22 matches
Mail list logo
