[READY] Implement partial sorting

[micbou]

This PR adds an optional parameter to the CandidatesForQueryAndType and FilterAndSortCandidates functions that limits the number of candidates returned by these functions. The candidates are partially sorted according to this limit (i.e. only the n smallest candidates are sorted).

The current behavior is unchanged: identifiers are still limited to 10 by default and there is no limit for other kind of candidates. The plan is to add a new setting (e.g. g:ycm_max_num_candidates) to limit the number of non-identifier candidates. 50 seems a reasonable choice.

The PartialSort function implements the following strategy:

• if the number of elements to sort is less than 1024 or represents less than 1/64 the total number of elements, use std::partial_sort;
• otherwise, use std::nth_element then std::sort on the nth smallest elements.

This heuristic was obtained by comparing the performance of these 3 algorithms:

• std::sort;
• std::partial_sort;
• std::nth_element + std::sort.

They were tested on randomly generated strings of 20 characters with different values for the total number of strings and the number of strings to sort. Here is the graph of an experiment with 2¹⁰ random strings:

The observation is that, when the number of elements to sort is small, std::partial_sort outperforms other algorithms while std::nth_element + std::sort always beats std::sort alone in other cases.
This experiment can be reproduced by checking out this branch and running the script plot_bench.py (the matplotlib package must be installed to draw the graph):

./plot_bench.py --bench bench.log

The number of elements can be modified by editing this variable.

Finally, here are the benchmark numbers obtained with these changes on my config. Conclusions are:

• no performance loss when all candidates are returned;
• a ~36% speedup in sorting and filtering identifiers with max_min_identifier_candidates default value.
• when limiting the number of non-identifier candidates to 50, sorting and filtering non-identifier candidates is ~5% faster if candidates are not already stored in the repository and ~24% if they are.

---

This change is 

[bstaletic]

Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

cpp/ycm/benchmarks/IdentifierCompleter_bench.cpp, line 55 at r1 (raw file):

BENCHMARK_REGISTER_F( IdentifierCompleterFixture, CandidatesWithCommonPrefix )
    ->RangeMultiplier( 1 << 4 )
    ->Ranges( { { 1, 1 << 16 }, { 10, 10 } } )

The { 1, 1 << 16 } part is the range itself. What does the { 10, 10 } pat mean?

---

cpp/ycm/benchmarks/PythonSupport_bench.cpp, line 56 at r1 (raw file):

    state.ResumeTiming();
    FilterAndSortCandidates( candidates, "insertion_text", "aA",
                             state.range( 1 ) );

What does state.reange( 1 ) evaluate to in this case?

---

Comments from Reviewable

[micbou]

Reviewed 2 of 12 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

cpp/ycm/benchmarks/IdentifierCompleter_bench.cpp, line 55 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

The { 1, 1 << 16 } part is the range itself. What does the { 10, 10 } pat mean?

{ 10, 10 } is the range for the maximum number of candidates returned by CandidatesForQuery. It's identical to { 10 } (in other words, we only test the value 10) but Google benchmark doesn't accept a single value for a range.

---

cpp/ycm/benchmarks/PythonSupport_bench.cpp, line 56 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

What does state.reange( 1 ) evaluate to in this case?

0 when the ranges are { { 1, 1 << 16 }, { 0, 0 } } and 50 when { 1, 1 << 16 }, { 50, 50 } }.

---

Comments from Reviewable

[codecov-io]

Codecov Report

Merging #825 into master will decrease coverage by <.01%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master     #825      +/-   ##
==========================================
- Coverage   94.82%   94.82%   -0.01%     
==========================================
  Files          79       79              
  Lines        5374     5389      +15     
  Branches      168      169       +1     
==========================================
+ Hits         5096     5110      +14     
  Misses        231      231              
- Partials       47       48       +1

[Valloric]

I'm sorry, but the PR description isn't at all clear to me. I've read it twice now and I still have no clue what this PR is trying to accomplish. :D It might have just been a long day at work, but still, could this be clarified? Perhaps with some examples.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bstaletic]

@Valloric Let me try to explain.

Current situation

Identifier completer - returns 10 candidates.
All other completers - return everything they can, no hard limit set.

Sorting - std::sort() always.

This pull request

Identifier completer - returns 10 sorted candidates.
Other completers - return up to 50 sorted candidates. Reasoning: User doesn't care for thousands of identifiers, only the most likely "few". Few is 50 by default, controllable with g:ycm_max_num_candidates.

Sorting - Partially sort those 50 (by default) candidates following this algorithm.

• If there are less than 1024 candidates use std::partial_sort().
• If the number of wanted candidates (50) is less than (total number of possible candidates)/64 use std::partial_sort()
• In all other cases use std:nth_element() then std::sort(). - This branch is what isn't tested according to codecov.

This results in much faster sorting of candidates.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[micbou]

Sorry for being unclear. The idea is that, since the maximum number of identifiers returned to the user is (by default) 10, we don't need to sort all of them but only the 10 smallest. This is significantly faster when the number of identifiers is big. The same can be done for non-identifier candidates if we limit them too. The rational in limiting them is that, in addition to improving performance, it's not particularly useful to return thousands of candidates. Users will never go through all of them.

@bstaletic did a good summary except that the PR doesn't actually set the maximum number of non-identifier candidates returned to the user to 50 but give the possibility to limit this number by adding an optional parameter to the FilterAndSortCandidates function. Setting the limit to 50 is subject to discussion and should be done in a separate PR.

---

Reviewed 9 of 12 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[bstaletic]

@micbou Taking another look at the code, the optional argument determining the max number of candidates is initialised defaulting to 0. with 0 having a special meaning of returning everything without any limitation on the number of returned candidates.

I really think we shuld default to some other value.

---

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[micbou]

The default is 0 to keep the current behavior which is to return all the non-identifier candidates. If we decide to limit the number of candidates, we should set the max_candidates parameter in the Python layer like we currently do for identifiers.

---

Reviewed 1 of 12 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks broke.

---

Comments from Reviewable

[bstaletic]

Fair point. Now why is codecov still reporting the same coverage after adding another test in r2 of this PR?

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[micbou]

Because the test only sorts the 2 smallest identifiers while it should sort at least 1024 if we want the other part of PartialSort to be covered. I am rewriting the test.

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[bstaletic]

With the updated test this is .

---

Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[micbou]

New test improved coverage. This is ready.

---

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[Valloric]

Thanks for the extra clarification folks! I get what this is trying to do now. Seems like a sensible change. Minor inline comments, but otherwise .

---

Review status: 12 of 13 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

cpp/ycm/Utils.h, line 191 at r4 (raw file):

// Shrink a vector to its sorted |num_sorted_elements| smallest elements. If
// |num_sorted_elements| is 0 or more than the vector size, sort the whole

"or more" -> "or larger"

---

cpp/ycm/Utils.h, line 210 at r4 (raw file):

  // alone in other cases.
  if ( max_elements <= std::max( static_cast< size_t >( 1024 ),
                                 nb_elements >> 6 ) ) {

IMO the right shift is too clever. Just use / 64 and the compiler will transform it for you into the shift. It will perform the same but it will be easier to read.

---

Comments from Reviewable

[micbou]

Reviewed 1 of 1 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

cpp/ycm/Utils.h, line 191 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

"or more" -> "or larger"

Done.

---

cpp/ycm/Utils.h, line 210 at r4 (raw file):

Previously, Valloric (Val Markovic) wrote…

IMO the right shift is too clever. Just use / 64 and the compiler will transform it for you into the shift. It will perform the same but it will be easier to read.

Done.

---

Comments from Reviewable

[Valloric]

Thanks!

@zzbot r=bstaletic

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[zzbot]

📌 Commit 1e00ddd has been approved by bstaletic

[zzbot]

⌛ Testing commit 1e00ddd with merge f180d07...

[zzbot]

💔 Test failed - status-travis

[zzbot]

☀️ Test successful - status-travis
Approved by: bstaletic
Pushing f180d07 to master...