[libc-commits] [PATCH] D111624: [libc] automemcpy - result analyzer

[Clement Courbet via Phabricator via libc-commits]

courbet added a comment.

As discussed, this is complex enough to warrant unit tests.



================
Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:11
+// - Reads one or more json files.
+// - If there are several runs for the same function and distribution, picks the
+//   median throughput (aka `BytesPerSecond`).
----------------
You have not defined `distribution in this file yet`. Give a pointer ?


================
Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:195
+    auto &Values = Pair.second;
+    std::sort(Values.begin(), Values.end());
+    const double MedianValue = Values[Values.size() / 2];
----------------
`std::nth_element`, given that you're not using the values for anything else ?


================
Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:211
+
+// For normalize the function's throughput per distribution and assign a
+// normalized score to the function for a particular distribution.
----------------
Normalize ?


================
Comment at: libc/benchmarks/automemcpy/lib/ResultAnalyzer.cpp:160-162
+  StringMap<double> Throughputs; // Median of samples per distribution
+  StringMap<double> Scores;      // Normalized score per distribution
+  StringMap<Grade::GradeEnum> GradeAttribution; // Grade per distribution
----------------
gchatelet wrote:
> courbet wrote:
> > any reason not to group `{Throughput,Score,GradeAttribution}` to avoid the 3 parallel maps ?
> Yes, `Throughputs` is currently move constructed (see line 185).
> I can extract the distribution data into its own data structure with 3 fields, but then I'll have to iterate the map to construct `Throughputs` instead of moving it.
> I'll update the patch with the new version so you can judge.
I think it's better grouped.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111624