Page MenuHomePhabricator

[clangd] Use JSON format in benchmark requests reader
ClosedPublic

Authored by kbobyrev on Sep 12 2018, 3:25 AM.

Details

Summary

After FuzzyFindRequest JSON (de)serialization was introduced, it should replace ad-hoc fuzzy-find request parsing implemented in the IndexBenchmark driver.

Diff Detail

Event Timeline

kbobyrev created this revision.Sep 12 2018, 3:25 AM
ilya-biryukov added inline comments.Sep 12 2018, 7:41 AM
clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
21 ↗(On Diff #165051)

We don't need the usings, just shorten the name on usage sites (usages are inside namespace clangd)

39 ↗(On Diff #165051)

Maybe read a JSON array from the file instead?
I.e. one would have to write [{/*request1*/}, {/*request2*/}, ..] instead of putting a request per line

Pros: we get valid json, can have prettified forms too.
Cons: might be a bit harder to construct the json array. But don't think it's a big deal.

kbobyrev planned changes to this revision.Sep 12 2018, 8:59 AM
kbobyrev marked an inline comment as done.

Apparently, the parsing seems to be wrong; I should fix that first.

Apparently, the parsing seems to be wrong; I should fix that first.

Oh, yeah, just use llvm::json::parse

kbobyrev updated this revision to Diff 165110.Sep 12 2018, 10:02 AM
kbobyrev marked an inline comment as done.

@ilya-biryukov thanks! I was assuming that the StringRef constructor parses user-provided string :(

llvm::json::Array is used now.

ilya-biryukov added inline comments.Sep 13 2018, 12:05 AM
clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
45 ↗(On Diff #165110)

Return from function after error?

47 ↗(On Diff #165110)

Maybe declare this var inside the loop where it's used?

48 ↗(On Diff #165110)

Check that the parsed value is an array and report an error if not?

kbobyrev updated this revision to Diff 165209.Sep 13 2018, 12:30 AM
kbobyrev marked 2 inline comments as done.
kbobyrev added inline comments.
clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
45 ↗(On Diff #165110)

I thought that this should panic instead of returning empty Requests: otherwise it wouldn't be possible to detect problems in the benchmark driver in test, for example (it will just run benchmark over empty list of requests instead of doing something useful).

ilya-biryukov added inline comments.Sep 13 2018, 1:15 AM
clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
45 ↗(On Diff #165110)

Let's "panic" explicitly, e.g. with exit(1).
We're currently triggering undefined behaviour, not something we want to do.

kbobyrev updated this revision to Diff 165232.Sep 13 2018, 2:43 AM
kbobyrev marked 3 inline comments as done.
kbobyrev added inline comments.
clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
45 ↗(On Diff #165110)

Good point; fixed!

kbobyrev updated this revision to Diff 165234.Sep 13 2018, 2:50 AM

Rebase on top of rL342123.

kbobyrev updated this revision to Diff 165235.Sep 13 2018, 2:53 AM

Fix the diff after rebase.

kbobyrev updated this revision to Diff 165240.Sep 13 2018, 3:10 AM

Fix empty scope in requests.json: %s/"::"/""/g.

ilya-biryukov added inline comments.Sep 13 2018, 3:11 AM
clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
47 ↗(On Diff #165235)

If there was a json parsing error, we should consume it and print to the user, i.e.

llvm::errs() << "Error when parsing json requests file: " << llvm::toString(std::move(JSONArray));

The user-facing error when an object is not an array should probably be a separate one.

53 ↗(On Diff #165235)

Check for return value of fromJSON and also report an error if it failed?

kbobyrev updated this revision to Diff 165248.Sep 13 2018, 4:34 AM
kbobyrev marked 3 inline comments as done.
ilya-biryukov added inline comments.Sep 13 2018, 6:42 AM
clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp
48 ↗(On Diff #165248)

We should only call takeError() when JSONArray is an error. Not an array value should be handled separately.

BTW, can we add tests with invalid inputs for both of these cases?

56 ↗(On Diff #165248)

Maybe also output the request that failed to parse?
To help untangle the problem in case it happens.

kbobyrev updated this revision to Diff 165276.Sep 13 2018, 7:10 AM
kbobyrev updated this revision to Diff 165277.
kbobyrev marked 2 inline comments as done.

Fix a typo in test comments.

ilya-biryukov accepted this revision.Sep 13 2018, 7:18 AM

LGTM

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

NIT: technically, the parsing succeeded at this point, maybe change to something like "top-level value is not a json array"?

60 ↗(On Diff #165277)

NIT: s/parsing/deserializing?

This revision is now accepted and ready to land.Sep 13 2018, 7:18 AM
kbobyrev updated this revision to Diff 165280.Sep 13 2018, 7:22 AM
kbobyrev marked 2 inline comments as done.
This revision was automatically updated to reflect the committed changes.