Page MenuHomePhabricator

[clangd] Add index benchmarks
ClosedPublic

Authored by kbobyrev on Aug 22 2018, 2:21 AM.

Details

Summary

This patch introduces index benchmarks on top of the proposed LLVM benchmark pull.

Diff Detail

Repository
rL LLVM

Event Timeline

kbobyrev created this revision.Aug 22 2018, 2:21 AM
kbobyrev planned changes to this revision.Aug 22 2018, 2:22 AM
kbobyrev edited the summary of this revision. (Show Details)

The current diff is rather messy and it is also blocked by the parent revision (D50894). It is likely to change if the parent CMake structure is changed.

kbobyrev updated this revision to Diff 162381.Aug 24 2018, 8:09 AM

Rebase on top of parent patch.

kbobyrev planned changes to this revision.Aug 24 2018, 8:10 AM
kbobyrev edited the summary of this revision. (Show Details)Aug 24 2018, 8:17 AM
kbobyrev edited the summary of this revision. (Show Details)
kbobyrev updated this revision to Diff 163821.Sep 4 2018, 7:59 AM
kbobyrev edited the summary of this revision. (Show Details)
  • Rebase on top of new code
  • Simplify code structure and get rid of global state (except for two filenames coming from main())

The only problem now is that the generated output contains log strings (e.g. "Built DexIndex with estimated memory usage 63445392 bytes.", see example in the Summary), I should find a way to deal with that (since it's rather annoying). It's also the case for the dexplorer tool.

kbobyrev updated this revision to Diff 164414.Sep 7 2018, 7:01 AM

Sync with HEAD

Nice and simple :-) Looks good, just some details.

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
1 ↗(On Diff #164414)

rename? (it's not just dex now, right?)

21 ↗(On Diff #164414)

const char* (no globals with nontrivial constructors/destructors)

25 ↗(On Diff #164414)

these should be in ns clang::clangd::<anonymous> I think?

26 ↗(On Diff #164414)

This includes reading the file contents, which I'm not sure is a useful part of the BuildDex() benchmark.

You could consider splitting into loadIndex that takes content and loadIndexFromFile that takes a filename. The former would be more appropriate to time.

33 ↗(On Diff #164414)

comment?

65 ↗(On Diff #164414)

this seems a little weird - the extractQueriesFromLogs/buildDex seem to be agnostic to the index contents, but here we assume knowledge of it being LLVM.

The former model is nicer, and we can extend it by using the JSON representation to add more query attributes. Can we drop this function, and ArtificialQueries->Queries?

104 ↗(On Diff #164414)

remove dex if this isn't dex-specific?

132 ↗(On Diff #164414)

BTW this also counts *destruction* time. I think that's fine, though.

kbobyrev updated this revision to Diff 164848.Sep 11 2018, 4:54 AM
kbobyrev marked 6 inline comments as done.

Address few comments (not all of them for now, though).

kbobyrev planned changes to this revision.Sep 11 2018, 4:55 AM
kbobyrev updated this revision to Diff 164881.Sep 11 2018, 7:55 AM
kbobyrev marked 2 inline comments as done.

The only problem left is that I'm not sure how to run binary which is not under bin (IndexBenchmark) using llvm-lit.

The only problem left is that I'm not sure how to run binary which is not under bin (IndexBenchmark) using llvm-lit.

But it's a benchmark.. What use could be from running it as part of the _tests_?
For test-suite (lnt) integration, i'm not sure.

kbobyrev updated this revision to Diff 164887.Sep 11 2018, 8:21 AM

Find a hacky workaround to call IndexBenchmark binary.

sammccall accepted this revision.Sep 11 2018, 8:26 AM

The only problem left is that I'm not sure how to run binary which is not under bin (IndexBenchmark) using llvm-lit.

But it's a benchmark.. What use could be from running it as part of the _tests_?
For test-suite (lnt) integration, i'm not sure.

Same as any other binary that isn't itself a test - make sure it works :-)
(Mostly just that it doesn't crash)

clang-tools-extra/test/clangd/benchmark-dummy.test
1 ↗(On Diff #164887)

i'd call this index-tools.test - it's a good integration test for global-symbol-builder, better when we add dexp to the test to verify the output.

This revision is now accepted and ready to land.Sep 11 2018, 8:26 AM

The only problem left is that I'm not sure how to run binary which is not under bin (IndexBenchmark) using llvm-lit.

But it's a benchmark.. What use could be from running it as part of the _tests_?
For test-suite (lnt) integration, i'm not sure.

Same as any other binary that isn't itself a test - make sure it works :-)
(Mostly just that it doesn't crash)

Then you don't actually care about the measurements, do you?
It would be a great idea to add --benchmark_min_time=0.01 so it does not actually try to measure anything good.

kbobyrev updated this revision to Diff 164895.Sep 11 2018, 8:43 AM

Add --benchmark_min_time=0.01 to prevent testing time increase.

lebedev.ri accepted this revision.Sep 11 2018, 11:43 PM

Add --benchmark_min_time=0.01 to prevent testing time increase.

Thanks!

kbobyrev edited the summary of this revision. (Show Details)Sep 12 2018, 12:45 AM
kbobyrev updated this revision to Diff 165023.Sep 12 2018, 12:50 AM
kbobyrev marked an inline comment as done.
Closed by commit rL342026: [clangd] Add index benchmarks (authored by omtcyfz, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.