-
Notifications
You must be signed in to change notification settings - Fork 769
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
[READY] Add FilterAndSortCandidates benchmarks #818
[READY] Add FilterAndSortCandidates benchmarks #818
Conversation
Codecov Report
@@ Coverage Diff @@
## master #818 +/- ##
=======================================
Coverage 94.78% 94.78%
=======================================
Files 79 79
Lines 5373 5373
Branches 170 170
=======================================
Hits 5093 5093
Misses 233 233
Partials 47 47 |
Reviewed 3 of 5 files at r1. cpp/ycm/benchmarks/IdentifierCompleter_bench.cpp, line 50 at r1 (raw file):
Are you sure that not testing lower ranges is a good idea? Hypoteticly, we could optimize for huge numbers of candidates, but accidentally hurt performance for small numbers of candidates. Comments from Reviewable |
This looks pretty good, thanks! I agree with @bstaletic's point though; we should ensure we look at smaller numbers of candidates as well. Review status: 3 of 5 files reviewed at latest revision, 1 unresolved discussion. cpp/ycm/benchmarks/IdentifierCompleter_bench.cpp, line 50 at r1 (raw file): Previously, bstaletic (Boris Staletic) wrote…
I agree, this is a concern. Comments from Reviewable |
8766cd0
to
6ed6803
Compare
@zzbot r+ Reviewed 2 of 5 files at r1, 2 of 2 files at r2. Comments from Reviewable |
📌 Commit 6ed6803 has been approved by |
…staletic [READY] Add FilterAndSortCandidates benchmarks [The `CandidatesForQuery` method](https://github.com/Valloric/ycmd/blob/2575bdb8c83dd5ace3d6f3d72a3425fdf2c18f5e/cpp/ycm/IdentifierCompleter.h#L69) is not the only place where performance is critical. There is also [the `FilterAndSortCandidates` function](https://github.com/Valloric/ycmd/blob/2575bdb8c83dd5ace3d6f3d72a3425fdf2c18f5e/cpp/ycm/PythonSupport.h#L29) which is used in the Python layer to filter and sort candidates returned by completers other than the identifier one (while `CandidatesForQuery` is specific to the identifier completer). We should add benchmarks for this function too. It would be great for these benchmarks to be based on real usage (e.g. case 1 or 2 from ycm-core/YouCompleteMe#2668). This could be done by storing the candidates in a file that would be read by the benchmarks to get the candidates. However, this kind of file would be too big (> 1MB) to be committed to the repository. We would have to download the file from somewhere else to run the benchmarks. I didn't want to bother so I went for the same candidates as the `CandidatesForQuery` benchmark. Those candidates represent a corner case but, as long as we don't specifically target them, they should be good enough. Two benchmarks are added: one when the candidates are not already stored in the repository and another when they are. This is important because both situations arise in practice: when filtering and sorting the candidates for the first time, they are added to the repository (user pressing `<C-Space>`) then they are retrieved from the repository next times (user typing characters to filter out candidates). Here are the benchmark results on my configuration: ``` Run on (4 X 3504 MHz CPU s) ------------------------------------------------------------------------------------------------------------------ Benchmark Time CPU Iterations ------------------------------------------------------------------------------------------------------------------ IdentifierCompleterFixture/CandidatesWithCommonPrefix/1024 512467 ns 514442 ns 1122 IdentifierCompleterFixture/CandidatesWithCommonPrefix/2048 1144025 ns 1143845 ns 641 IdentifierCompleterFixture/CandidatesWithCommonPrefix/4096 2643405 ns 2659108 ns 264 IdentifierCompleterFixture/CandidatesWithCommonPrefix/8192 6263122 ns 6267897 ns 112 IdentifierCompleterFixture/CandidatesWithCommonPrefix/16384 13002049 ns 12535795 ns 56 IdentifierCompleterFixture/CandidatesWithCommonPrefix/32768 28741026 ns 28704184 ns 25 IdentifierCompleterFixture/CandidatesWithCommonPrefix/65536 60231116 ns 60840390 ns 10 IdentifierCompleterFixture_BigO 57.59 NlgN 57.96 NlgN IdentifierCompleterFixture_RMS 1 % 2 % PythonSupportFixture/FilterAndSortUnstoredCandidatesWithCommonPrefix/1024 4280306 ns 4680030 ns 150 PythonSupportFixture/FilterAndSortUnstoredCandidatesWithCommonPrefix/2048 9249671 ns 9186726 ns 90 PythonSupportFixture/FilterAndSortUnstoredCandidatesWithCommonPrefix/4096 18832170 ns 18373451 ns 45 PythonSupportFixture/FilterAndSortUnstoredCandidatesWithCommonPrefix/8192 38904981 ns 37266906 ns 18 PythonSupportFixture/FilterAndSortUnstoredCandidatesWithCommonPrefix/16384 78318612 ns 78000500 ns 9 PythonSupportFixture/FilterAndSortUnstoredCandidatesWithCommonPrefix/32768 158404771 ns 163801050 ns 4 PythonSupportFixture/FilterAndSortUnstoredCandidatesWithCommonPrefix/65536 317453915 ns 319802050 ns 2 PythonSupportFixture_BigO 4836.93 N 4891.16 N PythonSupportFixture_RMS 1 % 2 % PythonSupportFixture/FilterAndSortStoredCandidatesWithCommonPrefix/1024 542299 ns 530403 ns 1000 PythonSupportFixture/FilterAndSortStoredCandidatesWithCommonPrefix/2048 1264767 ns 1279153 ns 561 PythonSupportFixture/FilterAndSortStoredCandidatesWithCommonPrefix/4096 2724379 ns 2718199 ns 264 PythonSupportFixture/FilterAndSortStoredCandidatesWithCommonPrefix/8192 6263308 ns 6267897 ns 112 PythonSupportFixture/FilterAndSortStoredCandidatesWithCommonPrefix/16384 12994143 ns 12792082 ns 50 PythonSupportFixture/FilterAndSortStoredCandidatesWithCommonPrefix/32768 27466364 ns 27300175 ns 28 PythonSupportFixture/FilterAndSortStoredCandidatesWithCommonPrefix/65536 57326742 ns 56727636 ns 11 PythonSupportFixture_BigO 54.99 NlgN 54.45 NlgN PythonSupportFixture_RMS 2 % 2 % ``` We see that filtering and sorting is much slower when candidates are not already stored so this is something we need to consider when trying to improve performance. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/818) <!-- Reviewable:end -->
☀️ Test successful - status-travis |
The
CandidatesForQuery
method is not the only place where performance is critical. There is also theFilterAndSortCandidates
function which is used in the Python layer to filter and sort candidates returned by completers other than the identifier one (whileCandidatesForQuery
is specific to the identifier completer). We should add benchmarks for this function too.It would be great for these benchmarks to be based on real usage (e.g. case 1 or 2 from ycm-core/YouCompleteMe#2668). This could be done by storing the candidates in a file that would be read by the benchmarks to get the candidates. However, this kind of file would be too big (> 1MB) to be committed to the repository. We would have to download the file from somewhere else to run the benchmarks. I didn't want to bother so I went for the same candidates as the
CandidatesForQuery
benchmark. Those candidates represent a corner case but, as long as we don't specifically target them, they should be good enough.Two benchmarks are added: one when the candidates are not already stored in the repository and another when they are. This is important because both situations arise in practice: when filtering and sorting the candidates for the first time, they are added to the repository (user pressing
<C-Space>
) then they are retrieved from the repository next times (user typing characters to filter out candidates).Here are the benchmark results on my configuration:
We see that filtering and sorting is much slower when candidates are not already stored so this is something we need to consider when trying to improve performance.
This change is