[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.
More information about the libcxx-commits