Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default to using 80% of cores available (instead of 100%) for indexing #536

Closed
wants to merge 1 commit into from

Conversation

eklitzke
Copy link
Contributor

@eklitzke eklitzke commented Dec 2, 2019

This makes the behavior of ccls match the existing comment in config.hh saying that 80% of cores will be used for indexing by default. You can still use 100% of threads by specifying index.threads=0.

@MaskRay
Copy link
Owner

MaskRay commented Dec 3, 2019

It was the following before:

        // If the user has not specified how many indexers to run, try to
        // guess an appropriate value. Default to 80% utilization.
        g_config->index.threads =
            std::max(int(std::thread::hardware_concurrency() * 0.8), 1);

If there are 2 cores, then one will be wasted. #6 changed it to 100%. IMO we should just use all cores to make initial indexing as fast as possible. Why do you want to use 80%?

@aravind2612krishna
Copy link

I can see where @eklitzke is coming from. Using 100% by default makes other applications unresponsive when indexing is going on. It is after all meant to be a background index. I personally use half the hardware_concurrency for when indexing in background and full concurrency when doing an overnight build and index of my huge source code (through ccls commandline).

@eklitzke
Copy link
Contributor Author

eklitzke commented Dec 3, 2019

I have two open PRs that affect indexing; this one at #538. I think we should merge one of them (but not both).

This change will indeed be somewhat wasteful on hosts that only have two cores, but I think that's probably uncommon on most hardware nowadays (even ARM CPUs often have 4 cores now); and users can still select 2 indexing threads if they wish.

In comparison, #538 will continue to use all available cores for indexing but won't change the number of indexing threads. The difference is that in that PR the kernel is instructed to only run the the indexing threads when there are no other tasks that are available to run.

Here's a concrete example of how this might play out. Suppose you have an 8-core desktop, and you start indexing a large project with ccls. At the same time, you try to run a parallel build process, and the build tool (make, bazel, whatever) automatically uses all available cores (which is relativley common). With this change there would be 6 ccls indexing threads + 8 gcc/clang threads, so assuming there are no other tasks on the system ccls will get about 42.8% of the CPU time and the actual build will get the remaining 57% of the build time.

With the change in #538 there will be 8 ccls indexing threads and 8 gcc/clang threads. However, the kernel will not schedule any of the indexing threads as long as another process is ready to run. This means that the gcc/clang build will get close to 100% of the available CPU power and you won't observe much overhead due to ccls, but ccls indexing will take much longer.

Another use case to consider is you start indexing a large project with ccls, but instead of starting a build you use a web browser or check your email while the index is happening. In this case the 20% of leftover cores is probably sufficient so that your browser/email client don't get blocked by ccls processes, and same with the SCHED_IDLE change, so in this situation both would probably be identical from a user experience perspective. But the SCHED_IDLE change might let ccls index faster, as you probably don't actually need 20% of available cores to check email.

I think these are both feasible strategies; note that clangd uses the "background" threads approach: https://reviews.llvm.org/D53651

@MaskRay
Copy link
Owner

MaskRay commented Dec 3, 2019

This change will indeed be somewhat wasteful on hosts that only have two cores, but I think that's probably uncommon on most hardware nowadays (even ARM CPUs often have 4 cores now); and users can still select 2 indexing threads if they wish.

I think it is very common to read/modify code on a laptop. For example on my X1 Carbon nproc prints 2. 1 core off from the total of 2~5 cores will be very wasteful.

I think these are both feasible strategies; note that clangd uses the "background" threads approach: https://reviews.llvm.org/D53651

I did notice that change. I did not follow because the motivation was not stated in that patch.

With the change in #538 there will be 8 ccls indexing threads and 8 gcc/clang threads. However, the kernel will not schedule any of the indexing threads as long as another process is ready to run. This means that the gcc/clang build will get close to 100% of the available CPU power and you won't observe much overhead due to ccls, but ccls indexing will take much longer.

I agree that the gcc/clang build should be given priority.

In this case the 20% of leftover cores is probably sufficient so that your browser/email client don't get blocked by ccls processes, and same with the SCHED_IDLE change, so in this situation both would probably be identical from a user experience perspective.

You do notice the difference after setting pthread_setschedparam(..., SCHED_IDLE, ...), don't you?

I have two open PRs that affect indexing; this one at #538. I think we should merge one of them (but not both).

We should go forward with #538.

@MaskRay
Copy link
Owner

MaskRay commented Dec 28, 2019

Updated and merged #538

@MaskRay MaskRay closed this Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants