[PATCH] D53994: Fixing lower bound regression in certain situations.

Louis Dionne via Phabricator reviews at reviews.llvm.org
Thu Nov 1 14:27:30 PDT 2018


ldionne added a comment.

In https://reviews.llvm.org/D53994#1284525, @dyaroshev wrote:

> In https://reviews.llvm.org/D53994#1284436, @ldionne wrote:
>
> > I did not participate in the original review, but my preference would be for https://reviews.llvm.org/D52697 to be reverted until the cause of the regression is understood and fixed. Unless other maintainers disagree.
> >
> > I'm a bit uneasy with checking-in a fix to an issue that is not well understood -- this is the standard library, after all, and we run on all kinds of platforms in all kinds of environments. "It works on my machine" usually does not mean much.
>
>
> I'm very much behind you.
>
> Is there some standard way that llvm project measures performance? Could we use that?


We have the benchmarks in libcxx, but I don't know whether we have any kind of performance testing infrastructure that would e.g. run those benchmarks under a wide array of configurations and report regressions / improvements. I don't think we do, otherwise I might have heard of it by now.

For simplicity, I did a wholesale revert of https://reviews.llvm.org/D52697 (as r345893) even though that patch included some benchmarks that are probably useful. Feel free to re-submit bits that do not impact the library on their own, but I think the algorithm should only be modified once the regression you found is understood and worked around.


Repository:
  rCXX libc++

https://reviews.llvm.org/D53994





More information about the libcxx-commits mailing list