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

[Samuel Benzaquen via Phabricator]

sbenza added a comment.

In D53994#1319418 <https://reviews.llvm.org/D53994#1319418>, @dyaroshev wrote:

> In D53994#1319307 <https://reviews.llvm.org/D53994#1319307>, @sbenza wrote:
>
> > re resize+iota, I don't see why not. That code is not part of the measurements.
> >
> > As for why `0...n`, for integers it doesn't really what the values are. only that there are N distinct values. The initial ordering of those values is changed depending on the benchmark.
> >  Note that for strings we don't use consecutive values as the value itself matters for `operator<`. That is, the first mismatched char is the one that matters and "sequential" strings would not be good for that.
> >
> > For `lower_bound` you might want a different sequence, though. You might also want to check for values missing in the list. The way we do it for std::set is to have `0,2,...,N`. We then search for `2*i` for hits and `2*i+1` for misses.
>
>
> I see your point about n values but I'm not entirely sold.
>  There is a famous bug in quick sort implementation, for example, - not partitioning the duplicates.
>  BTW just ran a small benchmark, check out how much duplicates affect the performance of std::sort http://quick-bench.com/mq8UiBPBT5vmmS7FG-RmY11VriQ


There are various input sequences. One of them is all duplicates.
We could have one that is N% duplicates if you think it is useful.

> I also suspect that median of 5 type of algorithm and other similar "find value to partition" techniques could  depend on the distribution - but maybe not.
> 
> Anyways, this is minor - just was confused a bit.
> 
> Why do you think for lower bound it is beneficial to look for "not found elements"?

lower_bound, just like std::set/map will stop the search when they find the element, but will generally continue all the way if it is not there.
The access pattern is a little different when it is all misses.

> It never checks for equality (it should just call a partition_point but it gets ugly().
>  I can run these benchmarks but I'd suggest not to commit them if they don't show anything.
> 
> Do you have any other suggestions? Popular sizes, 32/64 bit integers? I usually just pick 1000 and smth big but I don't know, maybe there is a research.

No idea about recommended sizes. I would assume lower_bound is mostly bound by L1 <https://reviews.llvm.org/L1> cache and branch prediction.
It should have a behavior similar to std::set in that the higher "nodes" will be hotter than the leaves.
sort has interesting behavior because they change algorithms depending on size. Not sure if lower_bound does this. Maybe they switch to linear search for small sizes?

> PS:
>  For strings I would suggest maybe to also use URL;s of some kind.
>  Random string comparisons results in a first character, right? But for URLs you often hit a long common prefix.

For strings I just kept the logic that was there before my change.
I agree that strings with common prefixes are a known (common) bad case for lexicographical sorting. URLs, file names, YYYYMMDD dates, etc tend to have largish common prefixes.
imo, the point of the string benchmarks is to compare a cheap-to-compare-and-swap type vs an expensive-to-compare-and-swap type. Extra compares/swaps would have a larger effect in the string benchmark.


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

https://reviews.llvm.org/D53994