[PATCH] D42690: [XRay] fix 99th percentile lookups by sorting the array correctly

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: tools/llvm-xray/xray-account.cc:243-245
+  std::nth_element(Timings.begin(), Timings.begin() + Pct99Off, Timings.end());
+  R.Median = Timings[MedianOff];
+  R.Pct90 = Timings[Pct90Off];
----------------
pelikan wrote:
> dberris wrote:
> > I don't think this does the same thing as the original code did (or what it was intended to do).
> > 
> > std::nth_element doesn't actually sort the elements before the `n`'th element. It just guarantees that the element that would have been at the `n`'th position is there.
> > 
> > We still need the two other calls to std::nth_element that would find the correct median, 90pct, and 99pct.
> Ah, it only partially sorts them.  I mis-read that on the internet (first time I saw that function).  Sorry, fixed.
Okay, now the problem is that calls to nth_element may re-order the elements before the n'th element. :)

If you want to make this cleaner with less repetition (and less potential for error), you might consider breaking this out into a function that encapsulates the nth_element and lookup to return the actual value. Something like:

```
R.Median = findPercentile(Timings, MedianOff);
R.Pct90 = findPercentile(Timings, Pct90Off);
...
```

Then you can hide the nth_element call.


Repository:
  rL LLVM

https://reviews.llvm.org/D42690