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

[Martin Pelikán via Phabricator via llvm-commits]

pelikan 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];
----------------
dberris wrote:
> 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.
Hahaha, gentle reminder I should probably go home. :-)

Thanks, fixed so it looks more like it used to.  No need for special functions here IMHO.


Repository:
  rL LLVM

https://reviews.llvm.org/D42690