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

Fri Jan 10 12:15:06 PST 2020

abrachet added a comment.

This is a very big patch and I don't have too much time today so I haven't reviewed this all and I focused on llvm style for now. A lot of comments I only made once but apply throughout.

I know something was posted on the mailing list today and I'm guessing this was talked about offline before work started here but I wish this was done more incrementally and discussed on the lists before. This is pretty hard to review as it is.

================
Comment at: libc/utils/HdrGen/CMakeLists.txt:1
+set(LLVM_LINK_COMPONENTS Support)
+
----------------
Why was this not needed before? Or maybe a better question why is it needed now?

================
Comment at: libc/utils/benchmarks/CMakeLists.txt:138
+# render-libc-{memcpy|memset|memcmp}-benchmark-{small|big}
\ No newline at end of file

----------------
newline

================
Comment at: libc/utils/benchmarks/json.cc:1
+//===-------- JSON serialization routines -----------------------*- C++ -*-===//
+//
----------------
`*- C++ -*` is not technically needed for source files only headers per the coding standards. Also for this and other files should be .cpp not .cc. Also I think these files are not named how they might usually be named, usually this would be something like JSON.cpp

================
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()) {
----------------
All of these assign to `Out` but we have `ErrorOr<T>` which is more ergonomic I think.

================
Comment at: libc/utils/benchmarks/json.cc:111
+        errc::io_error,
+        Twine("Can't parse BenchmarkLog, invalid value ").concat(String));
+  Out = *Parsed;
----------------
Maybe add `'` around `String`? It will be confusing if `String` can have spaces.

================
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]);
----------------
`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.

================
Comment at: libc/utils/benchmarks/json.cc:124-125
+  for (size_t I = 0; I < A->size(); ++I) {
+    auto E = fromJson((*A)[I], Out[I]);
+    if (E)
+      return std::move(E);
----------------
`if (auto E = fromJson(...)` and then remove the brackets with the for loop

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

================
Comment at: libc/utils/benchmarks/json.h:9
+
+#ifndef LLVM_LIBC_UTILS_BENCHMARK_JSON_H_
+#define LLVM_LIBC_UTILS_BENCHMARK_JSON_H_
----------------
I would remove the extra `_` at the end thats not common in llvm I find.

================
Comment at: libc/utils/benchmarks/json.h:14
+#include "libc_memory_benchmark.h"
+#include <llvm/Support/JSON.h>
+
----------------
Usually llvm includes are quote not bracket

================
Comment at: libc/utils/benchmarks/json_test.cc:119
+  const Study &Parsed = *StudyOrError;
+  EXPECT_THAT(Parsed, Equals(S));
+}
----------------
`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. 

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

================
Comment at: libc/utils/benchmarks/libc_benchmark.h:33-41
+#include <array>
+#include <chrono>
+#include <cstdint>
+
+#include "benchmark/benchmark.h"
+
+#include <llvm/ADT/ArrayRef.h>
----------------
```
#include "benchmark/benchmark.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallVector.h"
#include <array>
#include <chrono>
#include <cstdint>
```

Per [[ https://llvm.org/docs/CodingStandards.html#include-style | style guide ]],. Here and other places too.,

================
Comment at: libc/utils/benchmarks/libc_benchmark.h:53-55
+  NONE, // Don't keep the internal state of the benchmark
+  LAST, // Keep only the last batch
+  FULL  // Keep all iterations states, useful for testing or debugging
----------------
Comments should end with `.`

================
Comment at: libc/utils/benchmarks/libc_benchmark.h:85-89
+  RUNNING,
+  MAX_DURATION_REACHED,
+  MAX_ITERATIONS_REACHED,
+  MAX_SAMPLES_REACHED,
+  PRECISION_REACHED,
----------------
Usually enumerations are not all capitalized.

================
Comment at: libc/utils/benchmarks/libc_memory_benchmark.h:149
+
+  template <class Generator> inline uint32_t operator()(Generator &G) {
+    return Distribution(G) * Factor;
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-inline-when-defining-a-function-in-a-class-definition

================
Comment at: libc/utils/benchmarks/libc_memory_benchmark.h:172
+  template <class Generator>
+  inline uint32_t operator()(Generator &G, uint32_t Size) {
+    const uint32_t MismatchIndex = MismatchIndices[MismatchIndexSelector(G)];
----------------
https://llvm.org/docs/CodingStandards.html#don-t-use-inline-when-defining-a-function-in-a-class-definition

================
Comment at: libc/utils/benchmarks/libc_memory_benchmark_test.cc:26
+  AlignedBuffer AB(0);
+  EXPECT_THAT(std::distance(AB.begin(), AB.end()), 0U);
+}
----------------
`llvm::distance(AB)`
This is another example, couldn't this be EXPECT_EQ(..., 0U)?

================
Comment at: libc/utils/benchmarks/memcpy.cc:22
+struct MemcpyContext : public BenchmarkRunner {
+  using FunctionPrototype = void *(*)(void *, const void *, size_t);
+
----------------
Does this need to be a function pointer and not `llvm::function_ref` or `std::function`?

================
Comment at: libc/utils/benchmarks/memcpy.cc:52
+        Options, PP, [this, Function, Size](ParameterType p) {
+          (*Function)(DstBuffer + p.DstOffset, SrcBuffer + p.SrcOffset, Size);
+          return DstBuffer + p.DstOffset;
----------------
The explicit dereference of a function pointer is not necessary

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72516