[libc-commits] [PATCH] D72516: [llvm-libc] Add memory function benchmarks

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/utils/benchmarks/json.cc:31
+template <typename T>
+static Error intFromJsonTemplate(const json::Value &V, T &Out) {
+  if (const auto &MaybeInt64 = V.getAsInteger()) {
----------------
abrachet wrote:
> All of these assign to `Out` but we have `ErrorOr<T>` which is more ergonomic I think.
The JSON framework relies on overloading to dispatch the serialization to the correct function, e.g.
```
Error fromJson(const json::Value &V, double &Out)
Error fromJson(const json::Value &V, std::string &Out)
Error fromJson(const json::Value &V, uint32_t &Out)
...
```
C++ can't overload on return value so I can't use `ErrorOr<T>` here.


================
Comment at: libc/utils/benchmarks/json.cc:123
+  Out.resize(A->size());
+  for (size_t I = 0; I < A->size(); ++I) {
+    auto E = fromJson((*A)[I], Out[I]);
----------------
abrachet wrote:
> `for (size_t I = 0, Size = A->size(); I < Size; ++I)`
> Also there was a thread a while ago considering that we move to `ssize_t` because apparently loops can be unrolled better with signed integers, do with that whatever you want.
> 
> Also you're doing something with `A` and `Out` you can try `llvm::zip` maybe.
> for (size_t I = 0, Size = A->size(); I < Size; ++I)
Done

> Also there was a thread a while ago considering that we move to ssize_t because apparently loops can be unrolled better with signed integers, do with that whatever you want.
I've heard the opposite argument. Could you point me to that thread?

> Also you're doing something with A and Out you can try llvm::zip maybe.
Done thx !


================
Comment at: libc/utils/benchmarks/json.cc:149-152
+      : O(V.getAsObject()),
+        E(O ? Error::success()
+            : createStringError(errc::io_error, "Expected JSON Object")) {}
+
----------------
abrachet wrote:
> Was this `clang-formats` doing?
Yes what's the problem here?


================
Comment at: libc/utils/benchmarks/json_test.cc:119
+  const Study &Parsed = *StudyOrError;
+  EXPECT_THAT(Parsed, Equals(S));
+}
----------------
abrachet wrote:
> `EXPECT_THAT` is used a lot when other `EXPECT_*` would probably do. This one is just `EXPECT_EQ(Parsed, S)`, no? I've never really used matchers too much so maybe I'm missing something. 
In that case no because we have to instruct gtest how the two structs are comparing equals (hence the `Equals` overrides above).

That said for the other ones yes they're only comparing strings so `EXPECT_EQ` works.


================
Comment at: libc/utils/benchmarks/libc_benchmark.cc:18
+#ifndef NDEBUG
+  report_fatal_error("Library was built as DEBUG. Timings may be affected.");
+#endif
----------------
abrachet wrote:
> Maybe `static_assert` is fine? But I'm dubious that this is really needed to be honest.
Moved it to `LibcMemoryBenchmarkMain.cpp` as a `static_assert`


================
Comment at: libc/utils/benchmarks/memcpy.cc:22
+struct MemcpyContext : public BenchmarkRunner {
+  using FunctionPrototype = void *(*)(void *, const void *, size_t);
+
----------------
abrachet wrote:
> Does this need to be a function pointer and not `llvm::function_ref` or `std::function`?
What would be the advantage of using `llvm::function_ref` or `std::function` over raw function pointer? I don't want the type to be erased here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72516/new/

https://reviews.llvm.org/D72516